Closed Potherca closed 1 month ago
Besides cleaning up the CSS, I'm also not a fan of the non-compliance for closing the menu. It looks like @andybrewer had some opinions as well in #21. This is nice so long as it's not opinionated regarding styling, etc.
Can we look into making the menu compliant and then I can take a sweep on removing CSS that was copy/pasted and isn't needed?
Thank you for the feedback!
Gives me some nice action points.
On revisiting this one weekend later, I realized there are actually two consecutive use-cases
I think that use-case 1. should be resolved first, to give a uniform way the menu should look on smaller (mobile) screens without any hiding.
If/When that has been resolved, it would be trivial to create a toggle button. Whether that should then be part of core MVP.css is still up for debate. With styling for smaller screens, it might not even be needed...
The smallest CSS I could come up with, in order to display the menu on smaller screens, is:
@media (max-width: 768px) {
nav {
flex-wrap: wrap; /* Allow the links to drop below the logo */
}
nav ul li {
width: calc(100% - 1em); /* Make sure each link has enough room to have a separate line */
}
nav ul li ul {
display: block; /* Make the sub-menus visible (as there is no "hover" on mobile) */
}
}
The result (as can be seen on https://codepen.io/potherca/pen/abgRpGQ) is:
Smaller screen | Even smaller screen |
---|---|
Various minor tweaks could be made, for instance:
nav ul li ul { border: none; box-shadow: none; }
nav ul li ul { position: static; }
nav ul li a { display: block; }
I've added these tweaks commented out in the linked CodePen, so they can be played around with.
@ChristopherBilg Do you think I've missed anything?
This is great. I agree that the first point is more important for MVP.css and that the second point, in my opinion, doesn't make sense to add unless it can be added in with the same level of seamlessness that you implemented the first point.
As far as this codepen goes, it looks great. I think for the minor tweaks, definitely the first for removing borders. I'm not in favor nor against the second minor tweaks, although as I play with the codepen, I'm wondering if we can make this functional for recursive depths of nested lists. I'm seeing distortion adding in another level.
Yeah, I just did a re-run with more (sub-)menu items and I think the nav ul li ul { position: static; }
will be a requirement in order to keep things working with larger menus.
This will cause a large menu. The question then becomes whether that is okay or that a max-height
and overflow-y:auto;
will need to be added. Not sure yet 🤔
Regarding shadows, without looks better IMHO.
With shadow | Without shadow |
---|---|
Anyway, that would give us:
@media (max-width: 768px) {
nav {
flex-wrap: wrap; /* Allow the links to drop below the logo */
}
nav ul li {
width: calc(100% - 1em); /* Make sure each link has enough room to have a separate line */
}
nav ul li ul {
border: none; box-shadow: none; /* Remove the shadow */
display: block; /* Make the sub-menus visible (as there is no "hover" on mobile) */
position: static; /* Make the menu-items stretch the whole width (excluding padding/margin */
}
}
Fantastic! Want to open this in a PR and I'll get it approved right away? I think not having a max-height for an MVP makes sense, and if user's want a max-height they can trivially add that in. What are your thoughts on that?
I think not having a max-height for an MVP makes sense
Agreed.
I've opened #124 to add the proposed styles.
As I mentioned in #21, having generic support for the menu on mobile would be most awesome.
So I had a stab at making a backward-compatible, CSS only, plug-and-play implementation.
It can be seen in action at https://contrib.pother.ca/MetroJS/examples/
(Make sure to resize the screen to less than 800px or increase the font-size x8.
Part of the CSS could be removed, as it is copy-pasted from existing mvp.css code.
The CSS is thus (click to toggle)
```css /* Menu on mobile devices */ :root { --menu-label:'Toggle Menu'; } nav > button:empty, nav > button[aria-controls] { display: none; } @media (max-width: 768px) { nav > ul { background: var(--color-bg); bottom: 0; display: none; height: fit-content; left: 0; margin: 0; max-height: 100%; overflow: auto; position: fixed; right: 0; top: 0; } nav > button:empty, nav > button[aria-controls] { cursor: pointer; display: inline-block; } nav > button:empty::before { content: attr(aria-label, var(--menu-label)); } nav > button:empty:focus, nav > button[aria-controls]:focus { opacity: 0; } nav:has(button:empty) > ul::before, nav:has(button[aria-controls]) > ul::before { /* FROM: a b, a strong, button, input[type="submit"] */ background-color: var(--color-link); border: 2px solid var(--color-link); color: var(--color-bg); /* FROM: a b, a em, a i, a strong, button, input[type="submit"] */ border-radius: var(--border-radius); display: block; font-size: medium; font-weight: bold; line-height: var(--line-height); padding: 1rem 2rem; /* Element specific */ content: attr(aria-label, var(--menu-label)); cursor: pointer; float: right; margin: 0.75rem 1rem 0.5rem 0; } nav:focus-within > ul, nav:has(button:empty:focus) > ul, nav:has(button[aria-controls]:focus) > ul { display: block; } nav ul li { width: calc(100% - 1em); } nav ul li ul { display: block; position: static; } } ```To activate it, a
<button>
needs to be added in the<nav>
. This can be an empty button:A
--menu-label
is added to allow easy changing the toggle button text.A fully fledge (almost) aria compliant button could also be used:
The only thing that doesn't comply is that the close button can not be tabbed to. Everything else "just works" as the menu is not opened unless tabbed to.
So... Thoughts?