refactor navbar to be menubar #3300

johrstrom · 2024-01-17T18:41:29Z

This refactors the navigation bar to follow the menubar pattern. I'll try to leave comments here and there for more description on the changes.

The example/documentation I'm following is here:
https://www.w3.org/WAI/ARIA/apg/patterns/menubar/examples/menubar-navigation/

@abujeda can you please take a look at how this affects customizing the navigation bar? There could be some areas I missed and/or adversely affected.

johrstrom · 2024-01-17T18:42:08Z

apps/dashboard/app/helpers/application_helper.rb

-  def nav_link(title, icon, url, new_tab: false, role: nil, data: nil)
+  def nav_link(title, icon, url, new_tab: false, data: nil)


all nav links have the same role, so i'm removing this bit (nobody is using it anyhow).

johrstrom · 2024-01-17T18:43:39Z

apps/dashboard/app/views/layouts/nav/_all_apps.html.erb

+  <a title="<%= t('dashboard.nav_all_apps') %>" class="nav-link"
+     role="menuitem" href="<%= apps_index_path %>"
+     <%= aria %>
+    >


I just feel like this is nicer than calling tag.a methods in partials. Aside from the actual addition of title and role, this is largely a cosmetic change to actually template out an anchor instead of programatically generating it.

johrstrom · 2024-01-17T18:45:33Z

apps/dashboard/app/views/layouts/nav/_link.html.erb

-    class: "#{local_assigns.fetch(:class, 'nav-link')} #{local_assigns.fetch(:role, nil)}",
+    class: "#{local_assigns.fetch(:class, 'nav-link')}",


The role was being added to the class for some reason? In any case, all these have the same role, so this is being removed from the nav_link api anyhow.

abujeda · 2024-01-17T18:48:28Z

Will take a look and do some local testing

johrstrom · 2024-01-17T19:22:09Z

Will take a look and do some local testing

Thanks, I'm working on fixing the tests now (because this breaks the structure)

johrstrom · 2024-01-17T21:47:33Z

There is absolutely some wonkiness if you supply a custom help_menu with the default nav_bar.

abujeda · 2024-01-18T09:52:12Z

apps/dashboard/app/views/layouts/nav/_group.html.erb

-<li class="nav-item dropdown" title="<%= group.title %>">
-  <a href="#" class="nav-link dropdown-toggle" data-toggle="dropdown" aria-haspopup="true" aria-expanded="false">
+<li class="nav-item dropdown" role="none">
+  <a href="#" class="nav-link dropdown-toggle" data-toggle="dropdown" aria-haspopup="true" aria-expanded="false" role="menuitem">


Should the a tag have the title attribute?

In local testing, there is no text appearing on hover over the menu title

I guess it could. screen-reader reads it the same either way. The ul under this has a title, but I see what you mean - there's no hover over.

I wonder what the value is in a hover-over that has the same text you see on screen? In any case - I'm happy to add it back as it doesn't seem to affect accessibility.

abujeda · 2024-01-18T10:03:58Z

There is absolutely some wonkiness if you supply a custom help_menu with the default nav_bar.

Not sure what the issue is. For me, with a custom help_bar and a custom help_menu looks ok.
If you show me what help_menu config you are using, I should be able to replicate.

johrstrom · 2024-01-18T14:44:51Z

Looks like it could be on resize - I opened my dev tools and it resizes incorrectly. Note the whitespace to the right. The config is below - something from a test case but I think really it's the resize that does it.

```yaml help_bar: - title: Help Menu links: - group: 'Help Menu Dropdown Header' - title: 'Menu Link' url: /menu/link' new_tab: false - title: Profile Link profile: profile1 - title: Docs Menu links: - group: 'Docs Menu Dropdown Header' apps: - sys/bc_osc_jupyter - title: Custom Help Link url: '/custom/link' icon: 'fa://desktop' - user - logout ```

johrstrom · 2024-01-18T15:16:07Z

Well, this may actually be off in the mainline. this is what it looks like in the mainline.

I think that's just how it's been rendering when there's no margin available between the nav_bar and the help_bar.

abujeda · 2024-01-18T15:32:52Z

apps/dashboard/app/views/layouts/application.html.erb

-        </ul>
-      </div>
+
+          <div class="ml-auto d-flex">


Regarding the navigation look and feel, If we change from:
<div class="ml-auto d-flex">
to
<div class="ml-auto navbar-collapse">

will vertically center the navigation items for the help bar, making the look and feel more consistent when there is no space

nevermind, the helpbar is left aligned with the navbar-collpase class. Looking into alternatives

This div is definitely the cause of the issues or at least the biggest cause.

There may be a way to figure out which element is where and apply an ml-auto (or mr-auto) to the appropriate element dynamically somewhere.

This seem to do the trick:
<div class="ml-auto d-flex align-items-center">

Yea I can add align-items-center to the outter most ul - I think what I really need is an invisible div that adds the space between the nav_bar and help_menu instead of wrapping the help_menu inside a div. That'll likely help me fix the tests too when they all share the same general structure.

May have fixed it, it seems like I tried this before, but it didn't work for whatever reason then...

In any case, just adding this div between the two seems to do the trick (instead of wrapping the help_menu in a parent div)
<div class="ml-auto"></div>

Changes look good locally 👍

Thanks for the help!

abujeda · 2024-01-18T15:42:59Z

apps/dashboard/app/views/layouts/application.html.erb

-    <% else %>
-      <a class="navbar-brand" href="<%= root_path %>"><%= @user_configuration.dashboard_title %></a>
-    <% end %>
+      <ul class="navbar-nav w-100" role="menubar">


Similar to the helpbar, to vertically center the logo:
<ul class="navbar-nav w-100 align-items-center" role="menubar">

abujeda · 2024-01-18T16:37:19Z

If you want, once you are happy with the changes, I can fix the tests.
I know that you are pushing to complete other issues for 3.1

johrstrom · 2024-01-18T17:25:14Z

If you want, once you are happy with the changes, I can fix the tests.
I know that you are pushing to complete other issues for 3.1

That would be fantastic! thank you so much! Yes I think I'm happy with the structure now. It's pretty uniform and appears to be OK.

That said - I'm not 100% sure the custom stuff renders exactly right, so there may be additional patches on that front.

abujeda · 2024-01-18T17:27:13Z

OK - will start working on fixing the tests

Fixes to navigation tests after accessibility improvements

refactor navbar to be menubar

b92bec0

osc-bot added the component/dashboard label Jan 17, 2024

johrstrom commented Jan 17, 2024

View reviewed changes

remove tabindex here

cd3b94a

johrstrom commented Jan 17, 2024

View reviewed changes

abujeda reviewed Jan 18, 2024

View reviewed changes

add title on the anchor too

4447463

abujeda reviewed Jan 18, 2024

View reviewed changes

fix margin here

61a4b66

abujeda and others added 2 commits January 18, 2024 22:57

Fixes to navigation tests after accessibility improvements

2d38b02

Merge pull request #3306 from hmdc/accessible-navbar-tests

68f5211

Fixes to navigation tests after accessibility improvements

HazelGrant approved these changes Jan 24, 2024

View reviewed changes

johrstrom merged commit 55bf3f0 into master Jan 24, 2024
20 checks passed

johrstrom deleted the accessible-navbar branch January 24, 2024 17:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor navbar to be menubar #3300

refactor navbar to be menubar #3300

johrstrom commented Jan 17, 2024

johrstrom Jan 17, 2024

johrstrom Jan 17, 2024

johrstrom Jan 17, 2024

abujeda commented Jan 17, 2024

johrstrom commented Jan 17, 2024

johrstrom commented Jan 17, 2024

abujeda Jan 18, 2024 •

edited

Loading

johrstrom Jan 18, 2024

abujeda commented Jan 18, 2024 •

edited

Loading

johrstrom commented Jan 18, 2024

johrstrom commented Jan 18, 2024

abujeda Jan 18, 2024

abujeda Jan 18, 2024 •

edited

Loading

johrstrom Jan 18, 2024

abujeda Jan 18, 2024

johrstrom Jan 18, 2024

johrstrom Jan 18, 2024

abujeda Jan 18, 2024

johrstrom Jan 18, 2024

abujeda Jan 18, 2024

abujeda commented Jan 18, 2024

johrstrom commented Jan 18, 2024

abujeda commented Jan 18, 2024

		def nav_link(title, icon, url, new_tab: false, role: nil, data: nil)
		def nav_link(title, icon, url, new_tab: false, data: nil)

		class: "#{local_assigns.fetch(:class, 'nav-link')} #{local_assigns.fetch(:role, nil)}",
		class: "#{local_assigns.fetch(:class, 'nav-link')}",

refactor navbar to be menubar #3300

refactor navbar to be menubar #3300

Conversation

johrstrom commented Jan 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abujeda commented Jan 17, 2024

johrstrom commented Jan 17, 2024

johrstrom commented Jan 17, 2024

abujeda Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abujeda commented Jan 18, 2024 • edited Loading

johrstrom commented Jan 18, 2024

johrstrom commented Jan 18, 2024

Choose a reason for hiding this comment

abujeda Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abujeda commented Jan 18, 2024

johrstrom commented Jan 18, 2024

abujeda commented Jan 18, 2024

abujeda Jan 18, 2024 •

edited

Loading

abujeda commented Jan 18, 2024 •

edited

Loading

abujeda Jan 18, 2024 •

edited

Loading