Navigation: Add overlay close button to icon toggle control #43067
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mikachan
added
[Type] Enhancement
mikachan
commented
Aug 8, 2022
| @@ -626,6 +626,7 @@ button.wp-block-navigation-item__content { | |||
| border: none; | |||
| margin: 0; | |||
| padding: 0; | |||
| text-transform: inherit; | |||
There was a problem hiding this comment.
This is to fix a separate issue I noticed where the text version of these buttons doesn't inherit the text-transform from the navigation block, e.g. if text-transform: uppercase is set on the navigation block, this wasn't being applied to the text label.
|
Size Change: +67 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
|
When the button shows text, do we really need the aria-label? |
carolinan
approved these changes
Aug 9, 2022
There was a problem hiding this comment.
Switching between the text and the icons work well.
I think the text transform is the only typography option that works for the text only button, and that is thanks to this fix.
The text color option works partially, for the "Menu" but not the "Close".
- Search for, or open a new issue where we can discuss what settings should work on the buttons.
|
Thanks for the review, @carolinan!
Good point. No, I don't think we do need them, I think it's best to remove them completely when the button is text. I've added some logic for this here.
It seems that the 'Submenu & overlay text' color works for the 'close' button. |
scruffian
force-pushed
the
add/nav-close-icon-toggle
branch
from
ff2ccfa to
f03641b
Compare
scruffian
force-pushed
the
add/nav-close-icon-toggle
branch
from
f03641b to
3d61803
Compare
scruffian
force-pushed
the
add/nav-close-icon-toggle
branch
from
3d61803 to
51c7b24
Compare
82 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
This PR includes the navigation overlay close icon in the existing
hasIcontoggle so that we can control whether the overlay 'close' link is an icon or not in the same decision as the 'open' link.Closes #42848.
Why?
In #36149, we added the option to replace the overlay menu icon with text. However, when the overlay is open, we still use an icon for closing the overlay, without providing the option to replace this with text as well.
How?
Building on the existing toggle for
hasIcon, I've added additional logic for the close button that works in the same way as the open button. In the Display block settings, I've visually added the close button to the right-hand side of the open button.Testing Instructions
Screenshots or screencast
Display control with icons enabled, before interaction:
Display control with icons enabled, after opening the icon control:
Display control with icons disabled:
