-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use the Button block inside of the Search block #39463
Conversation
Size Change: +1.12 kB (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
@mtias said:
To which I replied:
And updated this PR accordingly. The styles are now synchronized between the search block and the button block. Through trial and error I ended up using controlled inner blocks with |
@scruffian Ah yes, I didn't get the icon to render so I replaced it with a text for now. That looks like a detail I can fix with more tinkering though, my main problem is with the editable text. |
@adamziel I don't think I've ever done something like this, sorry 😅
The button block is a RichText, so we can't use a paragraph block inside it. Even if we could paragraph comes with its settings, we would've to disable them. To be honest, this looks like a lot of work just to render a block and don't use any of the settings. |
What if we just used the .wp-block-button class on the search button? |
Capturing two notes from my conversation with @gziolo: |
Related merged PR for toolbar controls is #33955. Both render controls explicitly marked in the parent to child blocks. In your case, you need the reverse behavior. Force the child block to render all the block controls for the parent and optionally inject some additional controls from the currently selected child block. |
…racting with the inner button block
I ditched the template locks – it seems to be a dead end. I can't think of any API that would us specify control which block features we want locked, and which ones we want to be interactive. The PRs linked by @gziolo do not fix the problem either, but they are a really good entrypoint to exploring how would the implementation look like. Importantly, we do want to see the Search block toolbar and inspector controls, not the Button block's ones. I started with an absolutely terrible hack to trick the editor into thinking the search block is selected even when the user clicks on the button block: export function selection( state = {}, action ) {
if (
action.clientId ===
document.querySelector( '.wp-block-button' )?.dataset?.block
) {
action.clientId = document.querySelector(
'.wp-block-search'
)?.dataset?.block;
}
switch ( action.type ) {
case 'SELECTION_CHANGE':
// ... It only works if you have a single search block in the editor, and it does a surprisingly good job: For the record, I don't think that updating the reducer this way is a good idea at all. I just wanted to see if the "selection swap" concept has any chances of succeeding – it seems like it does. What do you think about the idea? If it sounds reasonable, I will try to roll out a proper implementation along the lines of @gziolo's PR linked above. CC @draganescu @getdave @scruffian |
I also considered rendering the button block to an element tree inside of a |
action.clientId === | ||
document.querySelector( '.wp-block-button' )?.dataset?.block | ||
) { | ||
action.clientId = document.querySelector( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This really looks like a hack 😁 and not sure if there even is a proper implementation of this.
buttonUseIcon, | ||
// Must serialize these two, they are new objects each time and | ||
// keep triggering the effect | ||
JSON.stringify( colorProps ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is weird :)
@adamziel reading through the comments I noticed @scruffian 's suggestion got no answer. Is adding that class not doing much? |
I'd like to add that I believe this is a case where we should definitely not be using child blocks. When thinking about whether inner blocks is the correct mental model to apply to the elements of a block we should think about what child blocks afford: the ability to declare, move, copy, duplicate, reorder, delete on one hand, and the ability to set attributes locally on the other. Neither of this apply elegantly to this block:
When we look at the results we aim to unlock, we also have the icon only button, which has no representation through Inheriting styles from the generic |
I had a go at adding support for a button element: #40260 |
Now #40260 has merged I think we can close this. |
Description
This PR replaces the
<button />
element inside of the Search block for aButton
block. It is a step towards consolidating the CSS required to render the Search block inside of the search block (overview issue). There are two major downsides of using an HTML element instead of a block:The solution proposed here comes with its own set of challenges.
Most notably, it is unclear how to reconcile granular control over the button block with the search block attributes such as
isButtonPositionInside
orbuttonUseIcon
. It was simple with a controlled HTML<button />
, but with a Button block the user may setbuttonUseIcon
totrue
on the Search block and then click on said icon inside the Button block and remove it. Should it be disallowed? Should that icon be restored after settingbuttonUseIcon
tofalse
and then back totrue
?cc @mtias @scruffian @getdave @draganescu @gziolo @dmsnell