New Accessible Dropdown Nav Bar Component #478

danparnella · 2021-05-10T20:04:58Z

Provides a contained and simple to stand up accessible navigation bar with a menu of parent links and optional child links hidden within submenus.

Known Issues

If while in mobile view, a user opens some submenus, then closes the mobile menu and expands their browser to show the desktop view, the submenus will still be open and will need to be closed with the arrow buttons. We could allow for something to be passed down from the mobile menu button to trigger closing all submenus when the mobile menu is closed, but this seemed like a far enough out edge case (and a non-breaking issue) to avoid adding more complication to the component.

Author Checklist

Add unit test(s)
Update version in package.json (see the versioning guidelines)
Update documentation (if necessary)
Add story to storybook (if necessary)
Assign dev reviewer

- remove baseUrl and nav link util - use Link when local path - create more unique key/id without needing to pass in one - update tests

src/controls/dropdown-nav-bar.js

danparnella · 2021-05-13T17:31:32Z

.storybook/styles/components/dropdown-nav-bar/_dropdown-nav-bar.scss

+.dropdown-nav-bar.no-mobile {
+  @import 'dropdown-nav-bar-no-mobile';
+  @import 'dropdown-nav-bar-common';
+}
+
+.dropdown-nav-bar:not(.no-mobile) {
+  @import 'dropdown-nav-bar-common';
+}


This allowed me to use the same styles for both implementations and just change the $tablet sass variable for the no-mobile version.

Interesting! I'll defer to @marchwiany on best practices in using dynamic variables as this is pretty foreign to me.

For my own edification, would the following not perform a similarly sequenced import? Maybe in the compilation step the second block is executed before the first?

.dropdown-nav-bar.no-mobile { @import 'dropdown-nav-bar-no-mobile'; } .dropdown-nav-bar { @import 'dropdown-nav-bar-common'; }

.storybook/styles/base/_typography.scss

docs.md

src/controls/dropdown-nav-menu.js

danparnella · 2021-05-13T19:57:13Z

stories/controls/dropdown-nav-bar.story.js

+        <input
+          type="checkbox"
+          id="mobile-nav-button"
+          checked={isMobileMenuOpen}
+          onChange={() => {
+            setIsMobileMenuOpen(!isMobileMenuOpen)
+          }}
+        />
+        <label htmlFor="mobile-nav-button" className="mobile-menu">
+          <span />
+          <span />
+          <span />
+        </label>


Pulled in the mobile menu button used on AZ, but we might have a more up to date version to swap in.

Can you walk me through the decision to not include this as part of the component?

@marchwiany Do we typically use buttons or are we still mostly using the checkbox approach?

chawes13

Looking great so far! I have a handful of questions, so it might be helpful to schedule some time to go through them sync

chawes13 · 2021-05-20T00:37:20Z

.storybook/styles/components/dropdown-nav-bar/_dropdown-nav-bar-common.scss

@@ -0,0 +1,91 @@
+& {


I haven't seen this usage before! What does & signify when it's used at the top-level?

chawes13 · 2021-05-20T00:39:14Z

.storybook/styles/application.scss

@@ -64,6 +64,7 @@
 @import "components/search-results-and-filtering";
 @import "components/recruiter-search";
 @import "components/tabs";
+ @import "components/dropdown-nav-bar/dropdown-nav-bar";


Will this need to be ported over to the client template as well or will the styles automatically be applied when a user imports the component into their project?

chawes13 · 2021-05-20T00:43:29Z

.storybook/styles/components/dropdown-nav-bar/_dropdown-nav-bar.scss

+.dropdown-nav-bar.no-mobile {
+  @import 'dropdown-nav-bar-no-mobile';
+  @import 'dropdown-nav-bar-common';
+}
+
+.dropdown-nav-bar:not(.no-mobile) {
+  @import 'dropdown-nav-bar-common';
+}


Interesting! I'll defer to @marchwiany on best practices in using dynamic variables as this is pretty foreign to me.

For my own edification, would the following not perform a similarly sequenced import? Maybe in the compilation step the second block is executed before the first?

.dropdown-nav-bar.no-mobile { @import 'dropdown-nav-bar-no-mobile'; } .dropdown-nav-bar { @import 'dropdown-nav-bar-common'; }

chawes13 · 2021-05-20T00:47:16Z

.storybook/styles/components/dropdown-nav-bar/_dropdown-nav-bar.scss

+#mobile-nav-button {
+  position: absolute;
+  top: -9999px;
+  left: -9999px;
+
+  &:checked {
+    & ~ .dropdown-nav-bar .dropdown-nav-menu {
+      top: 0;
+      position: fixed;
+      overflow: auto;
+      display: block;
+      height: 100%;
+      width: 100%;
+    }
+
+    & ~ .mobile-menu {
+      span:nth-child(1) {
+        transform: translateY(8px) rotate(45deg);
+      }
+      span:nth-child(2) {
+        opacity: 0;
+      }
+      span:nth-child(3) {
+        transform: translateY(-8px) rotate(-45deg);
+      }
+    }
+  }
+}
+
+.mobile-menu {
+  position: absolute;
+  right: 15px;
+  z-index: 3;
+  cursor: pointer;
+
+  @include media($tablet) {
+    display: none;
+  }
+
+  span {
+    display: block;
+    margin: 4px auto;
+    height: 4px;
+    width: 25px;
+    background: #333;
+    transition: 0.5s;
+  }
+}


What's the distinction between these styles and the ones included in _dropdown-nav-bar-common?

docs.md

chawes13 · 2021-05-20T02:48:49Z

src/controls/dropdown-nav-menu-item.js

+  }
+  const menuItemOnBlur = () => {
+    if (hideSubmenuButtonBeforeFocus) {
+      // timeout needed to move from link to button without it disappearing


Could you expand on this a bit? What's moving from the link to the button? What is "it"?

chawes13 · 2021-05-20T02:57:44Z

src/controls/dropdown-nav-menu-item.js

+        {isExternalPath ? (
+          <a href={path} {...menuItemProps}>
+            {name}
+          </a>
+        ) : (
+          <Link to={path} {...menuItemProps}>
+            {name}
+          </Link>
+        )}


There might be a useful abstraction here. Something like IsomorphicLink (inspired by https://github.com/streamich/react-use/blob/master/src/useIsomorphicLayoutEffect.ts)?

function IsomorphicLink ({ path, ...rest }) { const isExternalPath = path.startsWith('http') // or whatever the logic ends up being return isExternalPath ? <a href={path} {...rest} /> : <Link to={path} {...rest} /> }

chawes13 · 2021-05-20T02:58:53Z

src/controls/dropdown-nav-bar.js

+ *
+ * @name DropdownNavBar
+ * @type Function
+ * @description A control component for navigating through multiple navigation menu items that can include dropdowns with submenu items along with a mobile view option


Can you document the expected keyboard functionality here? If you have any links to ARIA authoring practices or other resources that inspired you, that'd be helpful as well!

Is there any need for arrow key support? It looks like the authoring practices specifically call out the situation where we have a menuItem that also performs a function:

As well as some other keyboard requirements:

Reference: https://www.w3.org/TR/wai-aria-practices-1.1/#menu

There might be some room for interpretation here, but from the following quotes and this example, it seems like using tab to navigate between the items may not be appropriate?

When a menu opens, or when a menubar receives focus, keyboard focus is placed on the first item. All items are focusable as described in § 6.6 Keyboard Navigation Inside Components.

6.6 Keyboard Navigation Inside Components

As described in section § 6.1 Fundamental Keyboard Navigation Conventions, the tab sequence should include only one focusable element of a composite UI component.

Ah, that number 4 note is the pattern I had trouble finding! I had moved away from using the arrow navigation vs tab navigation as I wasn't sure that was a common pattern when the parent menu item was also a link, but this sounds pretty clear to me. I'm working on that refactor now.

chawes13 · 2021-05-20T03:03:31Z

src/controls/dropdown-nav-menu-item.js

+      })}
+      role="none"
+    >
+      <OutsideClickHandler onOutsideClick={closeDesktopSubmenus}>


Similar question as before - are there use cases for closing all submenus vs. just the relevant submenu? Or should all submenus always be closed here?

If only one submenu can be open at a time, does that suggest that the value captured should be submenuId and not submenuIds?

chawes13 · 2021-05-20T03:11:35Z

src/controls/dropdown-nav-menu-item.js

+  const menuItemProps = {
+    onFocus: menuItemOnFocus,
+    onBlur: menuItemOnBlur,
+    role: 'menuItem',


Should this be 'menuitem'? Reference: https://www.digitala11y.com/menuitem-role/

danparnella added 13 commits April 7, 2021 17:42

create all components, update to use actual button trigger

ac1c68a

various updates to clean up code

60ffbf6

update id/class names, add aria-label for menu

7b49707

code cleanup, allow for click and keyboard actions in mobile view

1efbe42

add any missing aria attributes

bbc67b1

modify naming, add a comment

f360128

default with buttons always shown, add option to hide before focus

d0a4703

grab the top most window to check for browser width

4b4a912

allow for no mobile breakpoint/styles, update naming

e048cd4

move id, add aria controls

e9a3f52

add tests

a69dd10

- update some docs

0f058ed

- remove baseUrl and nav link util - use Link when local path - create more unique key/id without needing to pass in one - update tests

change menuLabel to menuAriaLabel, update docs

e176440

dpikt temporarily deployed to lp-components-pr-478 May 10, 2021 20:05 Inactive

update version

c0c5f0b

danparnella temporarily deployed to lp-components-pr-478 May 10, 2021 20:09 Inactive

danparnella added 2 commits May 10, 2021 15:38

update docs

f2af2b7

build updated docs file

1d2222f

danparnella temporarily deployed to lp-components-pr-478 May 10, 2021 20:39 Inactive

danparnella commented May 10, 2021

View reviewed changes

src/controls/dropdown-nav-bar.js Outdated Show resolved Hide resolved

danparnella marked this pull request as draft May 10, 2021 21:18

pull mobile menu button out of component and add to stories

b3e5825

danparnella temporarily deployed to lp-components-pr-478 May 13, 2021 18:25 Inactive

danparnella commented May 13, 2021

View reviewed changes

update naming

f9f79de

danparnella temporarily deployed to lp-components-pr-478 May 13, 2021 19:54 Inactive

apply new naming to tests

c2658ec

danparnella deployed to lp-components-pr-478 May 13, 2021 20:00 View deployment

danparnella commented May 13, 2021

View reviewed changes

danparnella marked this pull request as ready for review May 13, 2021 20:01

danparnella requested a review from chawes13 May 13, 2021 20:01

chawes13 reviewed May 20, 2021

View reviewed changes

danparnella added 3 commits June 1, 2021 18:58

start refactor with updated tests

aa1e826

add more tests

3c77519

fix tests

a10e8cd

chawes13 linked an issue Sep 17, 2021 that may be closed by this pull request

Add NavBar component #391

Open

chawes13 marked this pull request as draft September 21, 2021 15:08

danparnella added 3 commits January 31, 2022 12:56

update tests

054ebdb

reorganize test file

cf7d3b4

update tests to pass where they should

1c26012

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Accessible Dropdown Nav Bar Component #478

New Accessible Dropdown Nav Bar Component #478

danparnella commented May 10, 2021 •

edited

Loading

danparnella May 13, 2021

chawes13 May 20, 2021

danparnella May 13, 2021

chawes13 May 20, 2021

chawes13 May 20, 2021

chawes13 left a comment

chawes13 May 20, 2021

chawes13 May 20, 2021

chawes13 May 20, 2021

chawes13 May 20, 2021

chawes13 May 20, 2021

chawes13 May 20, 2021

chawes13 May 20, 2021

chawes13 May 20, 2021

chawes13 May 20, 2021

danparnella Jun 1, 2021

chawes13 May 20, 2021

chawes13 May 20, 2021

New Accessible Dropdown Nav Bar Component #478

Are you sure you want to change the base?

New Accessible Dropdown Nav Bar Component #478

Conversation

danparnella commented May 10, 2021 • edited Loading

Known Issues

Author Checklist

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

chawes13 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

6.6 Keyboard Navigation Inside Components

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danparnella commented May 10, 2021 •

edited

Loading