Move theme location settings to navigation editor sidebar #29458
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] Feature
grzim
requested review from
adamziel,
draganescu,
kevin940726,
noisysocks,
talldan and
tellthemachines
as code owners
draganescu
previously requested changes
Mar 3, 2021
There was a problem hiding this comment.
Is this PR based on #29012 or why does it contain all the menu name things?
| @@ -19,6 +23,7 @@ export default function SaveButton( { navigationPost } ) { | |||
| isPrimary | |||
| onClick={ () => { | |||
| saveNavigationPost( navigationPost ); | |||
| saveMenuName(); | |||
There was a problem hiding this comment.
How are all these changes involving menu name related to menu locations?
There was a problem hiding this comment.
@draganescu It does say in the PR description:
Branched from ( #29012 )
But I think the base branch in github should be set to add/navigation-menu-name-editor so that all of those changes don't appear in this PR. Github will automatically change the base to trunk when that branch is merged.
There was a problem hiding this comment.
Hmm, this change is still here, but it was part of that other PR.
edit: oh, just realised there are still lots of conflicts to resolve with trunk.
There was a problem hiding this comment.
🤔 I think this change is still from #29012. The menu location change shouldn't require any changes to saving.
talldan
reviewed
Mar 3, 2021
There was a problem hiding this comment.
Functionality wise, this seems to be working pretty well. Nice work @grzim.
I think before a final review it'd be great if you could refer back to the design to get this more closely matching what @shaunandrews mocked up.
A few things I noticed from the design that are missing in the implementation:
- The Manage Locations Panel has a title.
- The button that launches the manage locations modal is full-width and isn't the primary button style
- The modal doesn't have a close button at the bottom
- This part is missing - Navigation Screen: Update Navigation block inspector #28864 (comment)
packages/edit-navigation/src/components/inspector-additions/manage-locations.js
Outdated
Show resolved
Hide resolved
packages/edit-navigation/src/components/inspector-additions/manage-locations.js
Outdated
Show resolved
Hide resolved
|
Tested this and for me (Safari) everything refreshed whenever I changed something about locations in the inspector and also in the new modal. |
|
Currently |
|
@grzim you can skip bolding that menu name for now, I think. Are you talking about where it says "currently using Footer links"? If so, we can iterate later on making it bold. |
|
Took this for a spin and found a few things:
|
|
Thanks a lot, @shaunandrews for a feedback
All of the other mentioned issues have been addressed in the last commit |
7 tasks
This was referenced Mar 19, 2021
|
I have added two PR solving the issues described above: @shaunandrews can you please confirm I should change how the navigation position names should be displayed (second bullet in the previous comment)? Thanks! |
|
I have tested this wile possibility by adding in TwentyTwentyOne register_nav_menus(
array(
'primary' => esc_html__( 'Primary menu', 'twentytwentyone' ),
'Primary' => esc_html__( 'primary menu', 'twentytwentyone' ),
'primary-p' => esc_html__( 'primary menu', 'twentytwentyone' ),
'footer' => __( 'Secondary menu', 'twentytwentyone' ),
)
);and for it I got in the classic menu editor this:
So @grzim has a point, menu location slugs and menu location names are NOT case sensitive. While there is no valid reason a theme would use this for a rational reason (having wither a I think it's best if we let the menu label show up as the theme registered the menu, including letter case. |
grzim
force-pushed
the
add/navigation-block-inspector-theme-location
branch
2 times, most recently
from
626bf27 to
272ce65
Compare
|
I have added an issue about the problem with rerender #30139 and resolved all conflicts. |
|
Size Change: +829 B (0%) Total Size: 1.41 MB
ℹ️ View Unchanged
|
talldan
changed the title
talldan
reviewed
Mar 24, 2021
| * | ||
| * @type {string} | ||
| */ | ||
| export const MENU_KIND = 'root'; |
There was a problem hiding this comment.
This file was also moved in trunk so shouldn't be showing as an addition.
talldan
reviewed
Mar 24, 2021
Comment on lines
5
to
16
| .block-editor-block-inspector__no-blocks, | ||
| .block-editor-block-inspector { | ||
| width: $sidebar-width; | ||
| background: $white; | ||
| border-left: 1px solid $gray-300; | ||
| position: fixed; | ||
| top: $grid-unit-40; | ||
| right: 0; | ||
| bottom: 0; | ||
| z-index: 1; | ||
| overflow: auto; | ||
| } |
There was a problem hiding this comment.
This was removed in trunk, it shouldn't have been brought back.
talldan
reviewed
Mar 24, 2021
| openModal, | ||
| closeModal, | ||
| } ) { | ||
| const menus = useSelect( ( select ) => select( 'core' ).getMenus(), [] ); |
There was a problem hiding this comment.
This was removed in #29906, so will also need to bring this change over here.
talldan
force-pushed
the
add/navigation-block-inspector-theme-location
branch
from
272ce65 to
ec154a7
Compare
talldan
force-pushed
the
add/navigation-block-inspector-theme-location
branch
from
ec154a7 to
7af12b7
Compare
talldan
approved these changes
Mar 24, 2021
There was a problem hiding this comment.
I had to fix quite a few incorrect merge conflict resolutions to get this to a point where it can be merged. It seems like some of the changes in the PR itself were also undone by resolving things in the wrong way (the manage locations dropdown was back in the header). I think I've fixed everything.
I've noticed one small issue in testing:
The edit buttons are mis-aligned. That can be tackled separately.
|
Thank you @talldan for your effort. I have mentioned in a comment, that I need to check for the regression before the merge, but I should have converted this PR into the draft to avoid merging. |
gziolo
added
[Type] Enhancement
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
Closes #28864
Branched from ( #29012 )
The aim of this PR is to change the placement of Manage Locations to imitate the mechanics from
/wp-admin/nav-menus.php?action=locations(see the screenshot).Manage Locations button has been moved from Header to sidebar. Additionally, checkboxes have been added. Checkboxes are the indicators if a selected menu is placed in a menu location. If a checkbox relating to menu location is selected it means a selected menu is bound to this location. By design - one menu can be in multiple locations.
How has this been tested?
Tested manually
In order to test:
manage locationsbuttonScreenshots
Types of changes
New feature
Checklist: