UnderlineNav2: Handle the case when container is too small to render any items

[broccolinisoup]

Closes #2752

Issue

UnderlineNav v2 was throwing an error when the container is too small to render any items. Although there was no item in the list, it was still trying to swap a menu item with a list item to keep the selected one always visible and it resulted an error.

Solution

We fixed the issue by making sure we only swap items to keep the selected one visible when there is at least 1 element on the list. When the screen is too small to display one list element with the more menu button, we pull all items in the menu.

Caveat

When we display only the menu button, the menu items are not visually accessible. And this is something we are aware of. This is a very edge case as a browser can get that much narrow only when the dev tool is open. We will make sure we address this when working on mobile design for UnderlineNav.

Screenshots

Kapture.2023-01-27.at.13.59.46.mp4

Video displays the browser is being resized to a smaller size when the dev tool is open on the right hand side. As the browser gets smaller, the list items are being pulled into an overflow menu and they disappears from the screen. When the browser size is too small to display one list item and the menu button, all items goes into the menu and the overflow menu button's text goes from "More" to "Menu". Then the "Menu" button is clicked to show the items but as there is not enough space, the menu items are visually not accessible. Then the browser is being resized to a larger size, the menu items popping out of the menu and are displayed as list items to confirm the original functionality is working as expected.

Merge checklist

• Added/updated tests
• Added/updated documentation
• Tested in Chrome
• Tested in Firefox
• Tested in Safari
• Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

[changeset-bot]

🦋 Changeset detected

Latest commit: b0d61f2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

size-limit report 📦

Path	Size
dist/browser.esm.js	89.59 KB (+0.09% 🔺)
dist/browser.umd.js	90.22 KB (+0.09% 🔺)

[joshblack]

🥳

[danielguillan]

I think that from both design and accessibility perspectives, we might want to avoid horizontal scrolling in this scenario too.

A solution could be be to move all items into a dropdown menu if the visible item + more dropdown don't fit in the available space. The dropdown could be labeled using the provided aria-label.

[broccolinisoup]

I think that from both design and accessibility perspectives, we might want to avoid horizontal scrolling in this scenario too.

A solution could be be to move all items into a dropdown menu if the visible item + more dropdown don't fit in the available space. The dropdown could be labeled using the provided aria-label.

Thanks @danielguillan! I was very unsure about it after discarding the scroll behaviour but I couldn't really think of any other solution while making sure to keep the selected item visible and out of the menu. Your suggestion sounds good! I also wonder how we could indicate the selected item when it is in the dropdown?

[broccolinisoup]

UnderlineNav-too-narrow.mp4

Video displays the browser is being resized to a smaller size. As the browser gets smaller, the list items are being pulled in to an overflow menu and they disappears from the screen. When the browser size is too small to display one list item and the menu button, the last is being pulled into the menu as well and the More Menu button text changes to "Items" from "More"

@danielguillan is this something close to what you were thinking? I also updated the button text to "Items" when we only display the menu btn because "More" didn't make sense to me but it is just trying really, let me know what you think 🙌🏻 I also updated the aria-label accordingly

[broccolinisoup]

@broccolinisoup Good point! Then we could stick with "Menu". What do you think?

Sounds good @danielguillan - I updated it with "Menu". For the visual users, they will see the button with the "Menu" text, for screen reader users, it will be announced ${ariaLabel} Menu

[broccolinisoup]

Hi @primer/accessibility 👋🏻 Would it be possible to get a review for our work here? This story should be a good one to test. I provided info on the description but please let me know if you would need further details. Thank you 🙌🏻

[inkblotty]

Based on our conversation in Office Hour, this is a usable component. Some of the semantics are a bit overkill, but still accessible.

The visibility problem at 150 px width is a non-issue and does not need to be resolved from an accessibility standpoint.

Link to the recording

@broccolinisoup - If we missed testing something you needed eyes on, please let us know (@github/accessibility-reviewers). We're still getting our process for async Office Hours down 🙇

[broccolinisoup]

Based on our conversation in Office Hour, this is a usable component. Some of the semantics are a bit overkill, but still accessible.

The visibility problem at 150 px width is a non-issue and does not need to be resolved from an accessibility standpoint.

Link to the recording

@broccolinisoup - If we missed testing something you needed eyes on, please let us know (@github/accessibility-reviewers). We're still getting our process for async Office Hours down 🙇

Thank you @inkblotty so much! I just watched the recording and this was my first time submitting an issue to accessibility office hours so there were lots I learnt from this experience and how I can provide a better context and question next time - I do appreciate your patience with me 🙏🏻

I am so sorry that I forgot to provide the storybook link in the description and I could have explain better what exactly I needed to have an accessibility review on. You covered everything I was concerned though - thank you so much. I wasn't too worried about the 150px screen size as I mentioned it on the caveat. Although, we don't officially support this size, for users who somehow happen to end up in this size, I was still curious to understand if having only a menu button would that be good solution in terms of accessibility. Maybe it is a little bit overthinking 🙂 You answered all my questions and thank you so much again 🙇🏻‍♀️

[inkblotty]

So glad to hear all your questions were answered, even though we were a bit confused. Let us know how we can help in the future 🙂

…

On Wed, Feb 1, 2023 at 4:42 PM Armağan ***@***.***> wrote: Based on our conversation in Office Hour, this is a usable component. Some of the semantics are a bit overkill, but still accessible. The visibility problem at 150 px width is a non-issue and does not need to be resolved from an accessibility standpoint. Link to the recording <https://github.rewatch.com/video/xmanhqi2tjlg7vnk-accessibility-office-hours-feb-1-2023> @broccolinisoup <https://github.com/broccolinisoup> - If we missed testing something you needed eyes on, please let us know ( @github/accessibility-reviewers). We're still getting our process for async Office Hours down 🙇 Thank you @inkblotty <https://github.com/inkblotty> so much! I just watched the recording and this was my first time submitting an issue to accessibility office hours so there were lots I learnt from this experience and how I can provide a better context and question next time - I do appreciate your patience with me 🙏🏻 I am so sorry that I forgot to provide the storybook link in the description and I could have explain better what exactly I needed to have an accessibility review on. You covered everything I was concerned though - thank you so much. I wasn't too worried about the 150px screen size as I mentioned it on the caveat. Although, we don't officially support this size, for users who somehow happen to end up in this size, I was still curious to understand if having only a menu button would that be good solution in terms of accessibility. Maybe it is a little bit overthinking 🙂 You answered all my questions and thank you so much again 🙇🏻‍♀️ — Reply to this email directly, view it on GitHub <#2770 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADMMIM3VGYIHJK6EGNNYYRDWVLRDPANCNFSM6AAAAAATY36IOQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>