-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Try: hide search block label when contained #35688
Conversation
Size Change: +91 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
1d5aae4
to
7d26884
Compare
7d26884
to
707a2bb
Compare
3baea3a
to
8ee78b6
Compare
It also seems like block context is the right way to do it. I'd love to also default the search button to be inside, and icon-only, i.e. like this: If we can get to this point, we'll be in a pretty decent place, actually. One gotcha, though: as of the changes made in #34041, you'll likely be building your menu by first adding a few menu items, then adding a menu item and transforming it to a search block. When transforming, it appears that the block context is not leveraged to set these defaults: Is that something we can tweak, so the transform action also sets these defaults? Note, it's possible we need to fix that separately as part of #34041 and improvements to this transform behavior in general. |
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.
Thanks for working on this! I agree it's a great UI improvement for the Nav block, but have some reservations about the approach, as explained in the comment below 🙂
@@ -62,6 +62,10 @@ | |||
}, | |||
"customOverlayTextColor": { | |||
"type": "string" | |||
}, | |||
"searchBlockLabel": { |
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.
It doesn't feel right to have this as an attribute of the Navigation block, because it's not configurable within Navigation. Block attributes should be related to the block itself, usually as settings that affect the block or its children, but in this case we're trying to configure an attribute that only exists in the child block. The parent block shouldn't really need to know about it.
There's a precedent for this type of situation in Page List, where we check for a block parent directly from its edit function. This seems a more appropriate approach for the search label, and has the advantage that we can make it apply to other parent blocks without having to add attributes and context to each individual block (though I'm not sure if in this case we'd want this behaviour when Search is nested in any other block apart from Navigation)
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.
Update: #31670 changed Page List to extrapolate its parent from context, but that works because Page List already uses context from Navigation, which Search doesn't.
|
||
// Save a ref to this value, as it would become outdated | ||
// if any other blocks are added. | ||
const isNewBlockRef = useRef( isNewBlock ); |
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.
Not sure I understand why we need to use a ref here. Could you elaborate a bit on the problem?
Can we limit this to only the navigation block? Group was mentioned, but a visible label should be the first choice, hiding the label when inside one of the most used containers, the group, is negative for accessibility. |
Yes, the search block has good defaults and the change here should only be for when it's inserted in the navigation block. I can't think of a good example off the top of my head, but I do imagine that the mechanism could be useful for inserting other blocks in containers. |
Hi, everyone I've created PR with an alternative approach to this enhancement - #36026. |
Closed in favor of #36125 |
Description
Addressing #31127
Labels are shown by default, as established in block.json. This PR adds the possibility for parent blocks to define if they want a search bar label or not, by adding a
searchBarLabel
attribute, and passing it down through using Block Context.The flip is made:
showLabel
attribute istrue
(current default in block.json).How has this been tested?
Screenshots