Skip to content
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

fix(react): uishell headermenu blur handler #14616

Merged
merged 5 commits into from
Sep 12, 2023

Conversation

cordesmj
Copy link
Contributor

@cordesmj cordesmj commented Sep 8, 2023

Closes #14614

This PR modifies the HeaderMenu React component (part of the UIShell) such that when the menu loses focus, the menu is closed.

Changelog

Changed

  • onBlur handler for React HeaderMenu changed so that when the menu loses focus the menu is closed, unless focus is going to a child menu.

Testing / Reviewing

A basic test can be done on the storybook, as described in the referenced issue. That is, with this change, when tabbing through the submenu under "Link 4" in the storybook, upon pressing the Tab key from "Sub-link 3", focus passes to a link in the page content and the menu should close.

A more significant test can be done by adding a sub-submenu to the submenu. For example, in packages/react/src/components/UIShell/UIShell.HeaderBase.stories.js, after line 200, one could add the following:

              <HeaderMenu aria-label="Link 4" menuLinkName="Link 4">
                <HeaderMenuItem href="#">Sub-link 1</HeaderMenuItem>
                <HeaderMenuItem href="#">Sub-link 2</HeaderMenuItem>
                <HeaderMenuItem href="#">Sub-link 3</HeaderMenuItem>
              </HeaderMenu>

Tabbing around the menus should result in menus closing when losing focus, except when tabbing or clicking into child menus.

This will also facilitate extending the HeaderMenu to support a second level 'flyout' with the desired blur behavior.

image

@cordesmj cordesmj requested a review from a team as a code owner September 8, 2023 20:12
@netlify
Copy link

netlify bot commented Sep 8, 2023

Deploy Preview for v11-carbon-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit c8f6404
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/64fb8021ee1d130008a36a34
😎 Deploy Preview https://deploy-preview-14616--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify
Copy link

netlify bot commented Sep 8, 2023

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit c8f6404
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/64fb80211a1c9c000860b001
😎 Deploy Preview https://deploy-preview-14616--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify
Copy link

netlify bot commented Sep 8, 2023

Deploy Preview for v11-carbon-react ready!

Name Link
🔨 Latest commit aba156d
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/64ffad40588534000841b3f6
😎 Deploy Preview https://deploy-preview-14616--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify
Copy link

netlify bot commented Sep 8, 2023

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit aba156d
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/64ffad4084feea000874155f
😎 Deploy Preview https://deploy-preview-14616--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 ✅

@cordesmj
Copy link
Contributor Author

Hey @tw15egan, just noticed @andreancardona is on vacation this week. Is there some way I can request a review from someone else? I'm kind of eager to incorporate this change into my project, and I don't want to miss a version update. Thanks!

Copy link
Contributor

@andreancardona andreancardona left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just took a quick look thank you for the contribution @cordesmj !

@github-actions github-actions bot added this pull request to the merge queue Sep 12, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Sep 12, 2023
@tw15egan tw15egan added this pull request to the merge queue Sep 12, 2023
@cordesmj
Copy link
Contributor Author

Just took a quick look thank you for the contribution @cordesmj !

Thank you so much, Andrea! Didn't mean to disturb your vacation 🤦

Merged via the queue into carbon-design-system:main with commit 7d95369 Sep 12, 2023
18 checks passed
misiekhardcore pushed a commit to misiekhardcore/carbon that referenced this pull request Sep 18, 2023
Co-authored-by: Andrea N. Cardona <cardona.n.andrea@gmail.com>
misiekhardcore pushed a commit to misiekhardcore/carbon that referenced this pull request Sep 18, 2023
Co-authored-by: Andrea N. Cardona <cardona.n.andrea@gmail.com>
github-merge-queue bot pushed a commit that referenced this pull request Sep 20, 2023
* refactor(ComboBox): improve TS types

* chore(ComboBox): fix file formatting

* fix(react): uishell headermenu blur handler (#14616)

Co-authored-by: Andrea N. Cardona <cardona.n.andrea@gmail.com>

* fix: add isSelected prop to IconButton (#14600)

* fix: add isSelected prop to IconButton

* fix: update snapshot

* fix: change the HeaderMenuItem so it show docs in storybook (#14617)

Co-authored-by: TJ Egan <tw15egan@gmail.com>

* fix(contributors): remove duplicate contributors

* test(snapshot): update snapshots

---------

Co-authored-by: cordesmj <cordesmj@users.noreply.github.com>
Co-authored-by: Andrea N. Cardona <cardona.n.andrea@gmail.com>
Co-authored-by: Aziz Chebbi <60013060+azizChebbi@users.noreply.github.com>
Co-authored-by: TJ Egan <tw15egan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: React HeaderMenu blurHandler does not close menu as expected
3 participants