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

2.0.8: viewport dimension changes across breakpoint should close mobile menu, when appropriate #12

Closed
lkraav opened this issue Sep 16, 2021 · 11 comments

Comments

@lkraav
Copy link

lkraav commented Sep 16, 2021

We have specific things we want to do in CSS only on .open-mobile mode.

  • Put browser Responsive tool in mobile mode
  • Open menu
  • Switch browser Responsive tool > breakpoint width (desktop)
  • Observe .open-mobile class still applied

I feel like we could ResizeObserver here and programmatically close mobile menu when appropriate.

Your thoughts? Too much bloat for edge use case may be a concern, but maybe it's negligible.

@thednp
Copy link
Owner

thednp commented Sep 16, 2021

Honestly I did think about it, it didn't bother me.

However I was also thinking we could switch the open state from mobile view to desktop view and vice-versa on window resize, but in my view this is an unnecessary load of memory allocation.

A more simple approach I think is to close the submenu on window resize, while on close this event listener is also detached.

My priorities right now:

  • improve accessibility (aria-hidden, aria-expanded, aria-label) similar to Bootstrap's Dropdown / Collapse;
  • improve breakpoint styling based on [data-breakpoint="md"] like values instead of a $navbar_breakpoint SASS variable;
  • fix our RTL partial implementation (I believe I have it working nice on my end);
  • in relation to the above, rename SASS to better suit today's Bootstrap (EG: .navbar.left -> .navbar.start)
  • solve the issue you mention somehow
  • add some hide delay option
  • add event callbacks similar to Bootstrap components (EG: show.navbar =>(e) { e.target = menuItem; e.relatedTarget = someOtherItem }), maybe show, shown, hide & hidden
  • SASS some animation control: duration, translation, easing, etc
  • anything smart (small, usable, flexible) that won't increase the size of the Navbar component more than 1kb with all above combined.

Your 2cents.

@lkraav
Copy link
Author

lkraav commented Sep 16, 2021

A more simple approach I think is to close the submenu on window resize, while on close this event listener is also detached.

Probably would work fine for me ➕ 1️⃣

My priorities right now:

  • improve accessibility (aria-hidden, aria-expanded, aria-label) similar to Bootstrap's Dropdown / Collapse;

👍

  • improve breakpoint styling based on [data-breakpoint="md"] like values instead of a $navbar_breakpoint SASS variable;

👍

  • add some hide delay option

I'm not sure what this achieves for the user, didn't feel the need for delays in my own implementation.

  • add event callbacks similar to Bootstrap components (EG: show.navbar =>(e) { e.target = menuItem; e.relatedTarget = someOtherItem }), maybe show, shown, hide & hidden

👍 💯 I was looking for this. I'm looking to run special JS logic after we init, currently I do something like (works fine, but would be more "contained" in a callback):

        window.ACMEGroup = window.ACMEGroup || {};
        window.ACMEGroup.menuPrimaryNavbar = new Navbar('#menu-primary');

        /**
         * Un-flicker level 1 menu items.
         * Non-absolute position submenus force unwanted parent menu item width change.
         * Fixed width doesn't work, too spacious.
         */
        (function($) {
            $('#menu-primary-items > li').each(function() {
                $(this).css('max-width', $(this).width() + 'px');
            });
        })(jQuery || {});
  • SASS some animation control: duration, translation, easing, etc

Yeah I turned all of that off :)

  • anything smart (small, usable, flexible) that won't increase the size of the Navbar component more than 1kb with all above combined.

Your 2cents.

LGTM overall. I didn't get to building my own SASS here, so can't really speak to anything there that works well or doesn't.

@thednp
Copy link
Owner

thednp commented Sep 16, 2021

Also I forgot to mention: keyboard navigation :)

@lkraav
Copy link
Author

lkraav commented Sep 18, 2021

PS for event callbacks, I like how headroom.js has onXYZ properties https://wicky.nillia.ms/headroom.js/ to trigger anything you need on each event.

@thednp
Copy link
Owner

thednp commented Sep 18, 2021

I think the main <UL> will be the event.target property of the following events:

  • navbar.show - fires immediately on the specific triggering event, mouseenter or click on parent-icon, etc
  • navbar.shown - fires after on transitionend mostly, but only if that's the case
  • navbar.hide - fires immediately on the specific triggering event, mouseleave or click on parent-icon, etc
  • navbar.hidden - fires after on transitionend, but again only if that's the case

Other specs:

  • the reason why the main <UL> should be the event.target is to minimize the memory load, especially on large menus and playful end-users :D
  • show & hide events should be preventable
  • the <LI> that changes the visibility of it's child <ul class="subnav"> is the event.relatedTarget for all events
  • other events like onInit / onDisposed I would consider useless since our component exposes itself via the element target

@thednp
Copy link
Owner

thednp commented Sep 23, 2021

@lkraav I'm on the Navbar now.

  • Can you provide more info on the flickering?
  • Is it a backface-visibility: hidden fixable issue?
  • Can you provide a demo and steps to reproduce?

Thanks

@lkraav
Copy link
Author

lkraav commented Sep 23, 2021

Can you provide more info on the flickering?

This is about our specific implementation for .subnav, where we want to have position: static for level 1, so it would auto-expand the background behind it.

If you have Twitter or some other well known chat tool account, I can send you the production link there.

@thednp
Copy link
Owner

thednp commented Sep 23, 2021

@lkraav you don't have to share a production working draft, just give me a short video or drop a sample piece of HTML + CSS somewhere in codepen.

@lkraav
Copy link
Author

lkraav commented Sep 24, 2021

You don't have to share a production working draft, just give me a short video or drop a sample piece of HTML + CSS somewhere in codepen.

There's a Storybook but it isn't up to date. I sent you a follow request on Twitter y-day, it hasn't been accepted yet. I could DM you a URL there.

@thednp
Copy link
Owner

thednp commented Sep 25, 2021

Just to let you know:

  • events ({type: 'show.navbar', target: 'LI', relatedTarget: 'LI > UL,submenu'}), I'm still thinking about which elements should be which in the events objects
  • proper buttons for submenu toggle on mobile
  • aria-expanded implemented
  • SCSS sources improved
  • close submenus on window resize almost done, it should be mainly used to close submenus on mobile, it's a bit more complicated, but still tinkering with

@thednp
Copy link
Owner

thednp commented Oct 1, 2021

@lkraav feel free to test latest master and tell me what you think.

@thednp thednp closed this as completed Dec 28, 2021
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

No branches or pull requests

2 participants