Navigation Editor - Unification MenuSwitcher in UnselectedMenuState and Header #30349
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
grzim
added
[Type] Bug
grzim
requested review from
adamziel,
draganescu,
kevin940726,
noisysocks and
tellthemachines
as code owners
|
Size Change: +12 B (0%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
getdave
reviewed
Apr 21, 2021
There was a problem hiding this comment.
I tested this and can confirm that with this change the "unselected menu" state has the correct left/right padding of 8px which matches that in the menu "switcher".
However...
I'm not comfortable with applying this components-dropdown-menu__menu class purely for inheriting some styling when the target component is completely unrelated and not a dropdown. There's no telling what additional styles might be adding/remove from this [components-dropdown-menu__menu] class in future and this exposes the "unselected" menu component to regressions and introduces a maintenance burden.
I see two options:
- Create our own class which manually copies the (actually quite limited) set of styles we need to mirror the dropdown styles and apply this class to both the dropdown and the unselected menu state versions with high specificity. That way we can better ensure we are in control of the styling.
- Accept that the "unselected menu" state is not the same as the "switch menu" dropdown and style it slightly differently.
Personally, I'd say that #2 is a better approach. As things stand my first impression of the unselected menu state was that it needs some design anyway. It felt a bit "lost" in the middle of the page and it needs some instructional text and context.
I'd be interested in seeing what @critterverse thinks.
|
Update - the visuals for the "unselected menu" state could look a bit like those shown in the video in this Issue |
Completely agree with that. Applying our own styles might be ok. There's another thing I noticed, which is that the dropdown menu component is applying styles to menu groups, when perhaps menu group should implement its own styles. An option might be to move those styles from dropdown menu to menu group so that they're universal. I tried to check out all the usages of menu group components, and it seems like they're all used in dropdown menus, so I think this would be ok, however it's hard to say for sure without testing everything. |
|
My two cents here: I agree with @getdave that we would probably need to improve the design of the "Choose a menu to edit" screen, but in the meantime, I'd try to fix the padding of the second switcher since it doesn't look right. |
|
Closing this issue due to the Navigation Screen project being moved to an inactive status on the feature projects page in coordination with the project leads. (The developer documentation in the initial post are no longer accessible) If this work is picked back up, issues can be revisited and reopened as needed. Thanks for pitching in on this early feature so the wider WordPress project could benefit from the lessons learned! |
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.
Description
Edit: the issue is that the padding used on the menu switcher is different in the x2 places the menu switcher is used within the Navigation Editor:
Switch menubutton (top right).Delete menufrom the block settings in the right hand sidebar. You will then be asked to select a menu to editor.As
MenuSwitcheris wrapped inDropdownMenuinHeader, thecomponents-dropdown-menu__menucss class is added toNavigableMenu. InUnselectedMenuStatethe class was missing and it made the same component in these two places look slightly different - padding was missing.In order to make it look the same we can do one of the following:
components-dropdown-menu__menu- this would not be a scalable solutioncomponents-dropdown-menu__menu- it would be a preferable way from my perspective, but I remember @talldan pointed scss@extendshould be avoidedcomponents-dropdown-menu__menudirectly to theNavigableMenuelement - as it can be confusing in the code, I have added a comment to explain why this class is added thereHow has this been tested?
Switch Menuin the headerTypes of changes
Style fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).