Responsive Navigation #30047
Responsive Navigation #30047
Conversation
|
Size Change: +5.6 kB (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
|
I'll give this a good review Monday, and I'll help push polish and tweaks and we'll make this shine. But already from the demo video, this is looking incredible. Thank you! |
|
Took this one for a spin, this is what I see:
This is awesome! I'll see if I can push some things. |
|
One thing I noticed:
There are two empty |
jasmussen
force-pushed
the
responsive-navigation
branch
from
1bcf130
to
430b010
Compare
|
I rebased. Then I got the justifications to work again, editor and frontend. From digging in, it appears that MicroModal in order to function as intended, adds quite a few wrapper elements. This is understandable, I added classes to each so that they are easier and cleaner to target: Note that every wrapping element we can remove, we should. Less is more. But for now I'm assuming they are necessary, so working with them as is. |
vcanales
force-pushed
the
responsive-navigation
branch
from
9a4c0cd
to
973cfcb
Compare
jasmussen
force-pushed
the
responsive-navigation
branch
from
973cfcb
to
524f512
Compare
|
On the frontend, the close button is a
However the menu icon is a div:
Could the latter also be a button? |
|
Alright, I pushed a change so that you can now use the editor. A bit of work remains, and notably #29975 would fix a bunch of issues and benefit this PR in a rebase, but it's approaching a point where we can get a feel for both the editing and frontend interfaces, and start to course correct based on that.
I left two comments about extra empty paragraphs showing up, and the menu icon on the frontend being a button. The former is not that urgent, it's something we can always get to, but the latter would be nice, and I think it has some accessibility boons also. Overall, I think this is promising, I think that the option to allow the menu even on desktop, not just mobile, would be great. With a few more CSS refinements to the editing interface (I'll push more stuff as soon as I can), I think we'll be that much closer to evaluating whether it's a good experience or not, to edit menu items inside the mobile full-screen view. One thing already stands out: if we show menu items as a vertical list inside the menu, but a horizontal list by default, does it make sense to show the horizontal movers? The alternative seems very complicated, so it's something to ponder. |
|
Thank you for taking a look, and contributing! 🎉
I wasn't able to reproduce this. I tried T19, TT1, and TT1 Blocks. I also tried earlier commits — before hiding the tags with CSS — to no avail. Are there any other specific actions that I should follow to see those elements?
Yeah the extra markup is definitely something I'm keeping in mind — part of the learning process for this library will be to figure out which parts of the markup are essential, and which disposable. Regarding the justification of the menu, I'm seeing something similar to what you demoed:
Are we going to move those items at the top somewhere else, such as adding some padding? And, should we keep the orientation of the menu as defined by the user, or will be force vertical on mobile? |
That's curiuos... they disappeared! I'll remove the CSS for now.
💯
Yes, this aspect I'm still figuring out. I'm not quite sure what's the best path forward. For this PR it probably is to make a somewhat opinionated design. However the fact that right now, the same design applies for the menu itself, as the content inside the menu, means we don't really have that many layout options inside. Question: Instead of moving the contents of the navigation block inside the burger menu, could we duplicate it and hide one or the other depending on context? That way I could choose a horizontal nav for desktop, and put a vertical nav inside the menu on mobile. |
jasmussen
force-pushed
the
responsive-navigation
branch
from
da0b5e4
to
b34952b
Compare
|
Rebased. |
|
The empty P's came back for some reason:
So I'm adding the CSS back for now. This is my markup: |
|
One interesting thing I discovered — in my FSE theme, I already have a Navigation Menu block in my header template, and it picks up the menu feature excellently. But if I also include a navigation item in the page itself, for debugging or whatnot, it's the former menu that opens, regardless of where I click! Funny edgecase we probably don't have to worry about, but figured I'd mention. |
That was happening because I'm using the same ID in the server-side generate markup at the moment — fixed with f22efde.
We could duplicate it — right now we are not actually moving anything inside the burger menu. So, instead of printing just this markup, we could print that twice, or print different versions of it. This is also useful if we want to turn responsiveness off — we can print the |
|
Alright, I've pushed a ton of stuff, and I feel like this one is super close to being ready to go. Outside of the todo items you have — the toggle, etc, I would suggest these two are worth looking more into:
But user experiencewise, I think we're super close now:
I also made it so that color and style inheritance works:
The off-center navigation is caused by a separate bug that will be fixed once we rebase now that #29975 is merged. So where are we with the above? Essentially I figured that for this first version, we are going to be super opinionated about what's inside the navigation menu. Which means even if your menu is horizontal when not collapsed, it's going to be full screen and vertical when inside. This works now. I even made the block inert in the editor, when you are looking at the mobile menu contents. As you can see with the color stuff — at the moment it inherits the color shown by the parent navigation, OR it shows white background with black text. Addressing #29963 separately will benefit this work. All other styles are inherited and that works well too. Submenu indentation and styles is also something I mean to fix separately.
Per the opinionated design thoughts above, I think that for the time being we should embrace that the menu simply shows menu items in a prescriptive way. It allows us a great deal of simplicity with regards to making the burger menu be a toggle that just works, and we will always be able to revisit the decision in the future and expand. That said, the next step for this PR to land is to add that toggle in the first place, and to ideally look at how that markup can be simplified if at all possible. There are some chunks of CSS here that I find slightly hacky, and it'd be great to do without, things that tweak the pointer-events. But important to note: we should not be copying the menu just in order to make it separately editable — let's keep it simple for now. I.e. if I want to edit my menu, I have to only edit one menu. So copying would have to be something "behind the scenes" so to speak. Does that work for you? One question emerged from todays session: what do we do about submenus inside the modal menu? Right now they work fine, it's just a vertical menu as shown in the GIF above, so whatever we do should be a followup. But do we want to explode those menus or keep them as hover/focus? |
|
Oh, and I'll help with the rebase, it's probably going to be a bit gnarly, but I'll come back to this. |
This is definitely how it's working right now. The menu is not being copied — the one you see within the burger menu is the same as outside. The only JS magic going on is turning the
I'll be working on that toggle today. Even if we can't simplify the markup required for the modal, we will at the very least be able to print it conditionally — ie. only if responsiveness is on. |
jasmussen
force-pushed
the
responsive-navigation
branch
from
ecb1bd1
to
f9e1615
Compare
|
Rebased. Hurray, it includes the margin fix. Things are coming together. |
There was a problem hiding this comment.
This is looking promising @vcanales ✨ . I took this out for some manual testing, and wanted to highlight a few use cases:
- Let's make sure the preview for tablet/mobile works as expected:
- We currently can't add any items to a Navigation block when using a device with a small viewport:
mobile.mp4
- Triggering the touch event a few times on the 🍔 breaks the block:
|
Awesome review Kerry, thanks for taking a look. I'll see if I can help with the mobile preview.
This was actually intentional, though since you stumble on it I realize that of course you should be able to edit, and I'll revisit. The reasoning was that what you see inside the menu is our opinionated "flip a toggle and it works" view, and that you shouldn't be given the impression you can edit it when inside the menu, because in fact it's just a single menu, not a separate one for the menu. But the fact that you then can't edit it on mobile at all, obviously, is not great. So we can either: let you edit the menu on mobile again. That does mean you get horizontal movers even though we force the menu to be vertical there. We should probably do this for V1. Or, we can ponder how the editing experience of a menu is best on mobile, that may entail forcing you to use the List View entirely, but that requires a few additional features there, such as adding/removing items in the list view. This should probably be a V2. |
|
Gave this for a spin, and this is what I see:
Overall, this feels pretty dang solid in this latest iteration, and I think we should land it. Here's my line of thinking:
Because of all those reasons, because this works well, I'm going to green check this one so we can land it and get going with the followups. I would encourage additional quick sanity checks, if available, but I also think it's time. Nice work Vicente, and thanks for the help Nik! |
| } | ||
|
|
||
| // Hide empty paragraph tags caused by wpautop. | ||
| // @todo, need to not output them in the first place. |
There was a problem hiding this comment.
Changes were recently made to how empty paragraphs are displayed in the editor, so I thought it prudent to unresolve this thread and make sure these rules are really needed.
|
Great work here! Most of my comments above are nitpicks, but the one about hiding empty paragraphs is worth a second look. :) |
I haven't been able to reproduce this issue on my end, so I'm removing the rules. We'll be taking a look after merging anyway :) |
|
@mcsf Thanks for spotting all those details — there was some stuff leftover from the many changes this has gone through! I've addressed all of it. |
Co-authored-by: Miguel Fonseca <miguelcsf@gmail.com>
There was a problem hiding this comment.
This is very solid @vcanales ✨ Folks will really enjoy this one!
This is non-blocking, but I wanted to note a few areas we might want to tidy either here or in a follow up:
During Mobile Editing we can get confusing double models when previewing the menu + clicking on the global inserter:
modals.mp4
In the post-editor preview for mobile, shows all links instead of the hamburger:
 |
 |
|
Thanks for raising these, @gwwar! These may take a moment to fix, so I've created an issue to track them both, as they're very closely related. |
|
🔥 |
annezazu
added
the
[Focus] Accessibility (a11y)
Description
This PR introduces a mobile responsive Navigation Block. It add the markup necessary to display the Navigation Menu as a floating modal when the resolution of the site is below our mobile breakpoint.
Functionality in this PR depends on #29537 in order to add a library which enables toggling the visibility of the Menu, as well as creating an accessible experience. Here I am working under the assumption that this will be a possibility in the near future.
Next steps, and known issues
Testing steps
480px.Small demo
responsivenavdraft.mov