-
Notifications
You must be signed in to change notification settings - Fork 106
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
refactor navbar to be menubar #3300
Conversation
def nav_link(title, icon, url, new_tab: false, role: nil, data: nil) | ||
def nav_link(title, icon, url, new_tab: false, data: nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all nav links have the same role, so i'm removing this bit (nobody is using it anyhow).
<a title="<%= t('dashboard.nav_all_apps') %>" class="nav-link" | ||
role="menuitem" href="<%= apps_index_path %>" | ||
<%= aria %> | ||
> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
class: "#{local_assigns.fetch(:class, 'nav-link')} #{local_assigns.fetch(:role, nil)}", | ||
class: "#{local_assigns.fetch(:class, 'nav-link')}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Will take a look and do some local testing |
Thanks, I'm working on fixing the tests now (because this breaks the structure) |
There is absolutely some wonkiness if you supply a custom |
<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"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the a tag have the title
attribute?
In local testing, there is no text appearing on hover over the menu title
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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
```
|
</ul> | ||
</div> | ||
|
||
<div class="ml-auto d-flex"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nevermind, the helpbar is left aligned with the navbar-collpase
class. Looking into alternatives
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seem to do the trick:
<div class="ml-auto d-flex align-items-center">
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good locally 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the help!
<% else %> | ||
<a class="navbar-brand" href="<%= root_path %>"><%= @user_configuration.dashboard_title %></a> | ||
<% end %> | ||
<ul class="navbar-nav w-100" role="menubar"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the helpbar, to vertically center the logo:
<ul class="navbar-nav w-100 align-items-center" role="menubar">
If you want, once you are happy with the changes, I can fix the tests. |
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. |
OK - will start working on fixing the tests |
Fixes to navigation tests after accessibility improvements
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.