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.

Sign up for GitHub

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

Jump to bottom

fix(uishell): close HeaderMenu on blur #5003

Merged
merged 8 commits into from
Jan 13, 2020
Merged

fix(uishell): close HeaderMenu on blur #5003

merged 8 commits into from
Jan 13, 2020

Conversation

GregDevProjects
Copy link
Contributor

@GregDevProjects GregDevProjects commented Jan 10, 2020 •
edited
Loading

Closes #4930

{{short description}}

Close the HeaderMenu on blur, to prevent multiple HeaderMenus from being open at once

Changed

If the selected element that triggered the blur has the attribute of href=#, then it's another menu, so the expanded HeaderMenu should be closed

Testing / Reviewing

  1. cd packages/react && yarn storybook
  2. go to http://localhost:9000/iframe.html?id=ui-shell--header-base-w-navigation&viewMode=story#
  3. expand Link 4
  4. select Link 3

@GregDevProjects GregDevProjects requested a review from a team as a code owner January 10, 2020 15:37
@netlify
Copy link

netlify bot commented Jan 10, 2020 •
edited
Loading

Deploy preview for the-carbon-components ready!

Built with commit 4ef5aa9

https://deploy-preview-5003--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Jan 10, 2020

Deploy preview for carbon-elements ready!

Built with commit fb7f97b

https://deploy-preview-5003--carbon-elements.netlify.com

@netlify
Copy link

netlify bot commented Jan 10, 2020

Deploy preview for carbon-components-react failed.

Built with commit fb7f97b

https://app.netlify.com/sites/carbon-components-react/deploys/5e189a25e499e600080d2057

@netlify
Copy link

netlify bot commented Jan 10, 2020 •
edited
Loading

Deploy preview for carbon-elements failed.

Built with commit 4ef5aa9

https://app.netlify.com/sites/carbon-elements/deploys/5e1cedc2209bc500071dccaa

@netlify
Copy link

netlify bot commented Jan 10, 2020 •
edited
Loading

Deploy preview for carbon-components-react failed.

Built with commit 4ef5aa9

https://app.netlify.com/sites/carbon-components-react/deploys/5e1cedc2da0d3a00070a10d2

tw15egan
tw15egan previously approved these changes Jan 13, 2020
View reviewed changes
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, thanks for adding this! 👍

@tw15egan
Copy link
Member

Hmm, one thing I noticed is that tab navigation is now unable to navigate into the opened nav panel..

@tw15egan tw15egan dismissed their stale review January 13, 2020 19:45

Noticed something new

@GregDevProjects
Copy link
Contributor Author

thanks for the save @tw15egan! I added a check to ensure the element triggering the blur isn't one of the sub menu items before closing the HeaderMenu

tw15egan
tw15egan approved these changes Jan 13, 2020
View reviewed changes
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.

Changes look great! Thanks for updating so quickly

asudoh
asudoh approved these changes Jan 13, 2020
View reviewed changes
Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

LGTM 👍 - Thanks @GregDevProjects!

@asudoh asudoh merged commit e4a0ad3 into carbon-design-system:master Jan 13, 2020
@GregDevProjects GregDevProjects deleted the bug/4930 branch January 14, 2020 13:13
joshblack pushed a commit to joshblack/carbon that referenced this pull request Jan 23, 2020
@GregDevProjects @joshblack
fix(uishell): close HeaderMenu on blur (carbon-design-system#5003)
79fc14b
@joshblack joshblack mentioned this pull request Jan 23, 2020
Closed
@joshblack joshblack mentioned this pull request Jan 31, 2020
Closed
@joshblack joshblack mentioned this pull request Feb 25, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asudoh asudoh asudoh approved these changes

@tw15egan tw15egan tw15egan approved these changes

Assignees
No one assigned
Labels
status: ready to merge 🎉
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Multiple nav menus can be open at the same time, instead of closing onBlur
3 participants
@GregDevProjects @tw15egan @asudoh