Update navigation editor to support new submenu block #34281
Merged
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
talldan
added
[Type] Enhancement
talldan
requested review from
getdave,
kevin940726 and
tellthemachines
as code owners
7 tasks
|
Size Change: +19 B (0%) Total Size: 1.05 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks for addressing this so promptly! It's working well for existing submenus, but when adding a new one the inserter doesn't seem to be showing up in the submenu container.
There's an extra margin around the Submenu that's inherited from the general block styles, but that should be fixed once #34171 is merged.
Another thing I'm noticing is that in the conversion from Page List to editable links, the submenus are still being created with Navigation Link but I think that needs to be addressed in the parent PR - I forgot to make the necessary updates in the Page List block 😅
Good catch. I think the styles hide empty submenus by default, so that will require some tweaking. |
|
Ok, I've updated the Submenu PR to use Submenu in the Page List conversion, so this one should work correctly after rebasing. |
|
I tested this and like mentioned above there was no way to add an item in the submenu. |
tellthemachines
force-pushed
the
add/dropdown-block
branch
from
942bee8
to
cd044db
Compare
talldan
requested review from
ajitbohra,
spacedmonkey and
TimothyBJacobs
as code owners
talldan
force-pushed
the
update/nav-editor-for-dropdown-block
branch
from
5bd48ab
to
65e653a
Compare
7 tasks
|
Have updated this and gone a bit further to fix a lot of the broken navigation editor styles. Things should be much improved now. |
|
This fixes the broken nav screen rules for me, nice work:
Because of those regressions, I wonder if we should extract some of those fixes and land them separately (and I'd be happy to help here). Per the discussion in #18395, #33775 could be one option for configuring submenus. But there are some styling issues which while they shouldn't necessarily be too hard to fix might nevertheless make it miss the next plugin release:
I'll try and help here as best I can, but in case we'd like to fix the regressions in separately, that's also on offer. Let me know. |
7 tasks
|
I think it's ok to wait for the parent branch to be updated. There are a few CI issues as well to sort in that branch, but hopefully nothing major. |
tellthemachines
force-pushed
the
add/dropdown-block
branch
3 times, most recently
from
3567cde
to
be6def2
Compare
62 tasks
tellthemachines
approved these changes
Sep 10, 2021
| @@ -324,7 +324,7 @@ describe( 'Navigation editor', () => { | |||
|
|
|||
| // Select a link block with nested links in a submenu. | |||
There was a problem hiding this comment.
Should probably update the comment to "submenu block".
| @@ -30,8 +30,10 @@ | |||
| font-family: $default-font; | |||
|
|
|||
| // Increase specificity. | |||
| .wp-block-navigation-item { | |||
| .wp-block-navigation-item, | |||
| .wp-block-navigation-submenu { | |||
There was a problem hiding this comment.
I don't think we need this anymore because submenu blocks have both classnames as of #33918.
| @@ -55,13 +57,13 @@ | |||
|
|
|||
| // Menu items. | |||
| // This needs high specificity to override inherited values. | |||
| &.wp-block-navigation-item.wp-block-navigation-item { | |||
| &.wp-block-navigation-item.wp-block-navigation-item, | |||
| &.wp-block-navigation-submenu .wp-block-navigation-submenu { | |||
There was a problem hiding this comment.
This one shouldn't be needed either.
There was a problem hiding this comment.
Discovered I could remove the whole rule, it didn't seem to do anything as margin is set by a line further up 🤷
tellthemachines
force-pushed
the
update/nav-editor-for-dropdown-block
branch
from
16a6998
to
898b476
Compare
talldan
force-pushed
the
update/nav-editor-for-dropdown-block
branch
from
898b476
to
df75aef
Compare
This was referenced Sep 13, 2021
|
Isn't this a breaking change for classic menus? I mean, we don't expect Testing this with TT1 results in a pretty weird menu:
and some weird markup: |
|
@draganescu I don't think this changed anything, you could still create menu items without hrefs before this PR. Would you be able to share more details about what you're seeing? Unfortunately my dev environment needs to be fixed, so I'm unable to test. (When I last tested the submenu block still prompted with the URL Control upon creation. That might not be the case though if the implementation has changed in the submenu block.) |
|
Hmm, I was able to test and see what happens. For some reason the Link Control doesn't open straight away now, but if you click out and back into the submenu block it shows up. Seem like a bug with Submenu, it happens in the post editor too. I can take a look at a fix tomorrow, but an issue would be appreciated. |
What I mean is that in the classic navigation editor adding a custom link with no href is not possible. Initially. Hence I overreacted with the words "breaking change". Still it is possible to achieve an empty But now it's auto happening and I wonder if we increased our odds for themes to encounter surprise Maybe because it was hard to add a custom link without a href (although possible) many themes just expect href to be in menus. And what we do now with automatically making href-less anchors by adding a block is a very different thing. |
#18866 indicated that people have been using this as a roundabout way of creating non-link parents for submenus, so not allowing the behaviour at all is more of a breaking change 😅 I'll look into why the link control isn't opening at first; it should do so. |
|
#34798 should fix the issue. |
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.
Based on and merges into #33775
Description
Updates the navigation editor to support the dropdown block.
The changes are:
How has this been tested?
trunk.Types of changes
New feature (non-breaking change which adds functionality)
Checklist:
*.native.jsfiles for terms that need renaming or removal).