Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for menu on mobile #120

Closed
Potherca opened this issue Aug 24, 2024 · 7 comments · Fixed by #124
Closed

Support for menu on mobile #120

Potherca opened this issue Aug 24, 2024 · 7 comments · Fixed by #124

Comments

@Potherca
Copy link
Contributor

Potherca commented Aug 24, 2024

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)
/* 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:

<header>
    <nav>
        <button></button> <!-- ⬅️ This needs to be added -->
        <ul>
                <li>Menu Item 1</li>
                <!-- etc. -->
            </ul>

A --menu-label is added to allow easy changing the toggle button text.

A fully fledge (almost) aria compliant button could also be used:

<header>
    <nav>
        <button
            aria-controls="menu"
            aria-expanded="false"
            aria-label="Toggle Menu"
            onfocusin="this.setAttribute('aria-expanded', 'true')"
            onfocusout="this.setAttribute('aria-expanded', 'false')"
            onkeydown="if (event.keyCode === 27) this.blur()"
        >Open Menu</button>
        <ul id="menu" aria-label="Close Menu">
                <li>Menu Item 1</li>
                <!-- etc. -->
            </ul>

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?

@ChristopherBilg
Copy link
Collaborator

ChristopherBilg commented Aug 26, 2024

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?

@Potherca
Copy link
Contributor Author

Potherca commented Aug 27, 2024

Thank you for the feedback!

Full thought, in order (click to toggle)
  1. I'm also not a fan of the non-compliance for closing the menu

    That's been bugging me as well. The only two solutions I can think of that are pure CSS/HTML are:

    1. adding another button inside the menu
    2. using the input[type="checkbox"] & label as done in Responsive Menu for Mobile Screens #21.

    Or both? 🤷 I could code an example for either for easier comparison.

    Both solutions need HTML tags to be added. The question then becomes if that is acceptable. An/or which one is more acceptable.

  2. It looks like @andybrewer had some opinions as well in Responsive Menu for Mobile Screens #21.

    The major opinion seems to be:

    I think it might be a little too custom for this project

    Which motivated me to try to create something that is "of this project", reusing as much as possible of the MVP.css code, aesthetic and philosophy. However, It won't be possible to create something that just works with only the existing ul -> li HTML structure. Something will have to be added (i.e. an HTML tag) in order to have a hook to hang everything on.

    The more ARIA compliant, the larger that "something" becomes. Finding the right balance and an elegant solution is what makes this challenge fun, IMHO.

  3. This is nice so long as it's not opinionated regarding styling, etc.

    100% agree. I've tried to make it look as much in style as possible. It might need another round of work. For me, the main success criterion is that it has to look "in place" (i.e. not look out-of-place).

  4. Can we look into making the menu compliant

    I can set up examples for that, as mentioned above. I'll post links here when complete.

  5. then I can take a sweep on removing CSS that was copy/pasted and isn't needed?

    The copy/pasted CSS is only needed until nav:has(button:empty) > ul::before and/or nav:has(button[aria-controls]) > ul::before are added to the a b, a strong, button, input[type="submit"] selector (depending on what the final implementation will be). Although, thinking about it, if compliance it reached the ::before isn't needed so the point becomes moot.

    At this point all CSS present is needed, but I hope to make another pass and bring the code-size down

Anyway, thanks for the feedback again!

Gives me some nice action points.

@Potherca
Copy link
Contributor Author

Potherca commented Sep 2, 2024

On revisiting this one weekend later, I realized there are actually two consecutive use-cases

  1. Displaying the menu on smaller screen width
  2. Hiding the menu on smaller screen width and adding a mechanism to toggle the hidden menu

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:

  • Remove the border on sub-menus: nav ul li ul { border: none; box-shadow: none; }
  • Allow the sub-menus to have more width: nav ul li ul { position: static; }
  • Make the link area clickable across the entire width: 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?

@ChristopherBilg
Copy link
Collaborator

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.

image

@Potherca
Copy link
Contributor Author

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 */
  }
}

@ChristopherBilg
Copy link
Collaborator

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?

@Potherca
Copy link
Contributor Author

Potherca commented Oct 31, 2024

I think not having a max-height for an MVP makes sense

Agreed.

I've opened #124 to add the proposed styles.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants