548 navbar accessibility

[mononoken]

🔗 Issue

#548

✍️ Description

Note: Enthusiastic to hear cleaner, alternative approaches. I am not a fan of bootstrap and avoid it in my personal projects, so I may not be familiar with some better ways to overwrite some of this behavior.

This PR aims to make the navigation bar more accessibility friendly. Currently the right corner navbar on desktop view only shows if you hover the icon. This is terrible for keyboard users as it makes it potentially impossible for them to open it, and that's the only place where some of those links live!

This behavior is coming from media queries from the theme we are using. Some of the culprit rules are here:

@media (min-width: 1200px) {
  .navbar .dropdown-menu {
    display: block;
    opacity: 0;
    transform: translateY(20px);
    transition: all 0.3s ease-in-out;
    visibility: hidden;
  }
  .navbar .dropdown-menu-end {
    left: auto;
    right: 0;
  }
  .navbar .dropdown-menu-start {
    left: 0;
    right: auto;
  }
  .navbar .dropdown-submenu:hover > .dropdown-menu,
  .navbar .dropdown:hover > .dropdown-menu {
    opacity: 1;
    transform: scaleY(1);
    visibility: visible;
  }
}

The first approach I tried was to just overwrite the CSS rules I found from the theme. However, that approach did not work because I don't think there is a way to overwrite the JavaScript that the theme is utilizing as well based off of the viewport widths.

What I opted for instead was to create our own custom class for the menu and also use a simple Stimulus controller to toggle/collapse the menu.

What I do not like about this approach is the custom CSS. If we change the theme, we would have to update this CSS as well. I think the Stimulus controller is nice for our own control, but it may not fully mimic the behavior from BS.

Alternatives:

1. Instead of removing the theme classes and adding our own to the elements, we could keep the old theme classes, overwrite all the rules causing this behavior, and then also use our own JavaScript to enforce the toggling for the desktop view. Advantage to this approach is that we no longer need the custom css for our own class, but now we have custom css to overwrite all related rules. It may also be confusing having BS handling the JS for all the smaller viewports and then our own JS for the larger desktop one. We may have to enforce our own JS to only target the related elements for the desktop view too because the BS Javascript is going to still try to target them in the mobile view. That may cause some conflict.
2. Yuri on slack suggested reorganizing the HTML to try to avoid BS applying the theme's navbar behavior by taking the navbar menu button out of the "navbar":

<nav>
  <div class="navbar"> Navbar Link etc </div>
  <div>Icon Stuff</div>
</nav>

I think I might like this approach better, but I also am not fully satisfied with it too. Potential issues with it I see are:

1. We may have to fight the navbar to get it to look the same as it currently does, as the BS theme isn't expecting the menu button to be outside of it. For example, the second mobile menu will need some adjustment.
2. If we end up wanting more dropdowns in the navbar for any reason, not placed at the edge, this could potentially get even more convoluted.
3. Having a BS theme dictate how our elements are semantically arranged feels bad.

Fun fact, I noticed this hover behavior of the navbar has a test using a 5 second hover wait built into the test/system/user.rb test. Once @kasugaijin refactors the navbars so we use this one in both places, this new, no-hover behavior means we can shave 5 seconds off our test suite! lol

📷 Screenshots/Demos

In this clip, the left side is the live site (main) and the right side is this branch.
https://github.com/rubyforgood/pet-rescue/assets/81536479/23e4683b-872e-47e9-b104-eff73a45e23d

[mononoken]

@Developer3027 I just uploaded this PR for the issue, if you have a chance to look it over and share your thoughts as well.

[kasugaijin]

@mononoken this is a good thing to change up, though I do have some thoughts on it:

• I prefer we don't modify the default theme much, to keep things simple. However, in this case, this is improved accessibility for an important feature. So worth some investment in my opinion (thank you!). Some googling on UX best practices yields 'The hover approach is popular but click-to-open is the best way to ensure the menu works reliably and intuitively across all devices.' So, yes, let's go with the click.
• I am fine with your solution. I think adding some CSS and JS in this case is justifiable. Though, I would request we add comments to both so that it is obvious what they are doing i.e. overriding the theme functionality, and why.
• In your implementation, I need to click on the div element (circle) to open the menu, which is fine. I was expecting to be able to click anywhere else on the window to close the menu. Currently, I have to click the div element to close the menu.

[mononoken]

@mononoken this is a good thing to change up, though I do have some thoughts on it:

• I prefer we don't modify the default theme much, to keep things simple. However, in this case, this is improved accessibility for an important feature. So worth some investment in my opinion (thank you!). Some googling on UX best practices yields 'The hover approach is popular but click-to-open is the best way to ensure the menu works reliably and intuitively across all devices.' So, yes, let's go with the click.
• I am fine with your solution. I think adding some CSS and JS in this case is justifiable. Though, I would request we add comments to both so that it is obvious what they are doing i.e. overriding the theme functionality, and why.
• In your implementation, I need to click on the div element (circle) to open the menu, which is fine. I was expecting to be able to click anywhere else on the window to close the menu. Currently, I have to click the div element to close the menu.

@kasugaijin Good suggestion! I added clicking outside to close the menu. I also added the comments.

[Developer3027]

I agree with above. Will review this afternoon. Thanks!

[Developer3027]

After review, I like this approach. This does use a new css style set, however it follows convention, works like intended, and utilizes current theme attributes. The javascript in the Stimulus controller is straight forward and easy to read. Comments allow a clear understanding of what it does and the why is
intuitive.

I agree it needs to be understood why this was done and to take note of both the Stimulus controller and the styles set. Maybe consider a note in the read me? Is documentation being considered, maybe there?
@mononoken @kasugaijin

[kasugaijin]

After review, I like this approach. This does use a new css style set, however it follows convention, works like intended, and utilizes current theme attributes. The javascript in the Stimulus controller is straight forward and easy to read. Comments allow a clear understanding of what it does and the why is intuitive.

I agree it needs to be understood why this was done and to take note of both the Stimulus controller and the styles set. Maybe consider a note in the read me? Is documentation being considered, maybe there? @mononoken @kasugaijin

Documentation is an ongoing challenge in codebases. I think the further the documentation gets away from the code, the harder it can be to find and update, and the README might not be the most practical location for code decisions. Ideally, code speaks for itself, and comments add context. I think in this case, I am happy with the inline comments, as added by @mononoken.

[mononoken]

Thank you both!

Got the comment updated to ERB per Slack @kasugaijin . Think this should be good for final review.

[kasugaijin]

LGTM!