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(masthead): Esc to close Cloud Masthead #10555

Merged
merged 7 commits into from
Jun 15, 2023
Merged

fix(masthead): Esc to close Cloud Masthead #10555

merged 7 commits into from
Jun 15, 2023

Conversation

pjudge
Copy link
Contributor

@pjudge pjudge commented Jun 13, 2023

Related Ticket(s)

Closes #10174

Description

The Cloud masthead (and other mastheads) simple menus were not closing upon hitting Esc if focus was inside the submenu. This PR fixes that.

Changelog

Changed

  • Adds ability to close Cloud Masthead non-megamenu (and all non-megamenus) with Esc key, just as megamenus already do.

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Jun 13, 2023 •
edited
Loading

Deploy preview created for package React:
https://ibmdotcom-react.s3.us-south.cloud-object-storage.appdomain.cloud/deploy-previews/10555/index.html

Built with commit: b5b5b2390706b04e3510be72ec07f52ef8552cb3

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Jun 13, 2023 •
edited
Loading

Deploy preview created for package Carbon Web Components:
https://carbon-web-components.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/10555/index.html

Built with commit: b5b5b2390706b04e3510be72ec07f52ef8552cb3

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Jun 13, 2023 •
edited
Loading

Deploy preview created for package Web Components:
https://ibmdotcom-webcomponents.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/10555/index.html

Built with commit: b5b5b2390706b04e3510be72ec07f52ef8552cb3

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Jun 13, 2023 •
edited
Loading

Deploy preview created for package Web Components (React wrapper):
https://ibmdotcom-web-components-react.s3-web.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/10555/index.html

Built with commit: b5b5b2390706b04e3510be72ec07f52ef8552cb3

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Jun 13, 2023 •
edited
Loading

Deploy preview created for package "React (experimental)":
https://ibmdotcom-react-experimental.s3.us-south.cloud-object-storage.appdomain.cloud/deploy-previews/10555/index.html

Built with commit: b5b5b2390706b04e3510be72ec07f52ef8552cb3

@ibmdotcom-bot
Copy link
Contributor

Deploy preview created for package Web Components HTML Test Application:
https://ibmdotcom-web-components-html-test-upstream.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/10555/index.html

Built with commit: a13cba822db26cf7a0c3b049c800339169a44e29

@pjudge pjudge marked this pull request as ready for review June 13, 2023 19:24
@pjudge pjudge requested a review from a team as a code owner June 13, 2023 19:24
jkaeser
jkaeser approved these changes Jun 13, 2023
View reviewed changes
Copy link
Member

@jkaeser jkaeser left a comment

Choose a reason for hiding this comment

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

👏 This looks excellent. I've confirmed the fix works in both the web component packages' deploy previews. Great work!

If you're interested, I have a couple notes that won't functionally change anything but are some opportunities to make this a smidge more idiomatic:

  1. Instead of adding an event listener in the connectedCallback(), try using the @HostListener() decorator above the _handleKeydownTrigger method. See the _handleBlur method for an example. This way we're consistent with how we're handling event listeners.
  2. Since this event listener is being set on the custom element itself, we shouldn't need to set it on the <a> tag with the @keydown expression anymore.
  3. I noticed both the BXHeaderMenu._trigger and DDSMegamenuTopNavMenu._topMenuItem properties store references to the same thing: the top-level menu item. Seems like an opportunity to reduce some redundancy. Let me know if you want to talk about this one.

@jkaeser jkaeser added package: web components Work necessary for the IBM.com Library web components package package: carbon web components 👀 eyes needed labels Jun 13, 2023
@annawen1 annawen1 changed the title 10174: Esc to close Cloud Masthead fix(masthead): Esc to close Cloud Masthead Jun 14, 2023
@pjudge
Copy link
Contributor Author

pjudge commented Jun 14, 2023

@jkaeser Thank you so much for your thorough and thoughtful review! I've made the updates you suggested and when the build finishes, it's ready for a re-review 👍

jkaeser
jkaeser suggested changes Jun 14, 2023
View reviewed changes
Copy link
Member

@jkaeser jkaeser left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my feedback! Just one change needs reverting and I'd say we're good to go here.

packages/carbon-web-components/src/components/ui-shell/header-menu.ts Outdated Show resolved Hide resolved
@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Jun 14, 2023 •
edited
Loading

Deploy preview created for package "Web Components (experimental)":
https://ibmdotcom-web-components-experimental.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/10555/index.html

Built with commit: b5b5b2390706b04e3510be72ec07f52ef8552cb3

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Jun 14, 2023 •
edited
Loading

Deploy preview created for package NextJS Test Application:
https://ibmdotcom-nextjs-test-upstream.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/10555/index.html

Built with commit: b5b5b2390706b04e3510be72ec07f52ef8552cb3

annawen1
annawen1 approved these changes Jun 15, 2023
View reviewed changes
Copy link
Member

@annawen1 annawen1 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 @pjudge and @jkaeser!

@jkaeser jkaeser added Ready to merge Label for the pull requests that are ready to merge and removed 👀 eyes needed labels Jun 15, 2023
@jkaeser
Copy link
Member

jkaeser commented Jun 15, 2023

@annawen1 Looks like Percy failed to generate some screenshots and is blocking this merge. Are you able to resolve that, or do we need to reach out to the design team?

@kennylam
Copy link
Member

Looks like Percy failed to generate some screenshots and is blocking this merge. Are you able to resolve that, or do we need to reach out to the design team?

Fixed!

@kodiakhq kodiakhq bot merged commit de67bf8 into carbon-design-system:main Jun 15, 2023
kennylam pushed a commit to kennylam/carbon-for-ibm-dotcom that referenced this pull request Jun 22, 2023
@pjudge @kennylam
fix(masthead): Esc to close Cloud Masthead (carbon-design-system#10555)
178ed84 
### Related Ticket(s)

Closes carbon-design-system#10174 

### Description

The Cloud masthead (and other mastheads) simple menus were not closing upon hitting Esc if focus was inside the submenu. This PR fixes that. 

### Changelog

**Changed**

- Adds ability to close Cloud Masthead non-megamenu (and all non-megamenus) with Esc key, just as megamenus already do.
@kennylam kennylam mentioned this pull request Jun 22, 2023
Merged
IgnacioBecerra pushed a commit that referenced this pull request Jun 22, 2023
@kennylam @pjudge @kodiakhq
fix(masthead): Esc to close Cloud Masthead (#10555) (#10588)
06cd69d 
### Related Ticket(s)

Closes #10174 

### Description

The Cloud masthead (and other mastheads) simple menus were not closing upon hitting Esc if focus was inside the submenu. This PR fixes that. 

### Changelog

**Changed**

- Adds ability to close Cloud Masthead non-megamenu (and all non-megamenus) with Esc key, just as megamenus already do.

Co-authored-by: Pauline <pauline.judge@lullabot.com>
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
kennylam pushed a commit to kennylam/carbon-for-ibm-dotcom that referenced this pull request Dec 4, 2023
@pjudge
fix(masthead): Esc to close Cloud Masthead (carbon-design-system#10555)
388f1d2 
### Related Ticket(s)

Closes carbon-design-system#10174 

### Description

The Cloud masthead (and other mastheads) simple menus were not closing upon hitting Esc if focus was inside the submenu. This PR fixes that. 

### Changelog

**Changed**

- Adds ability to close Cloud Masthead non-megamenu (and all non-megamenus) with Esc key, just as megamenus already do.
kennylam added a commit to kennylam/carbon-for-ibm-dotcom that referenced this pull request Dec 4, 2023
@kennylam @pjudge @kodiakhq
fix(masthead): Esc to close Cloud Masthead (carbon-design-system#10555)…
d183084 
… (carbon-design-system#10588)

### Related Ticket(s)

Closes carbon-design-system#10174 

### Description

The Cloud masthead (and other mastheads) simple menus were not closing upon hitting Esc if focus was inside the submenu. This PR fixes that. 

### Changelog

**Changed**

- Adds ability to close Cloud Masthead non-megamenu (and all non-megamenus) with Esc key, just as megamenus already do.

Co-authored-by: Pauline <pauline.judge@lullabot.com>
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
kennylam pushed a commit to kennylam/carbon-for-ibm-dotcom that referenced this pull request Jun 11, 2024
@pjudge
fix(masthead): Esc to close Cloud Masthead (carbon-design-system#10555)
f010ffc 
### Related Ticket(s)

Closes carbon-design-system#10174 

### Description

The Cloud masthead (and other mastheads) simple menus were not closing upon hitting Esc if focus was inside the submenu. This PR fixes that. 

### Changelog

**Changed**

- Adds ability to close Cloud Masthead non-megamenu (and all non-megamenus) with Esc key, just as megamenus already do.
kennylam added a commit to kennylam/carbon-for-ibm-dotcom that referenced this pull request Jun 11, 2024
@kennylam @pjudge @kodiakhq
fix(masthead): Esc to close Cloud Masthead (carbon-design-system#10555)…
3c865df 
… (carbon-design-system#10588)

### Related Ticket(s)

Closes carbon-design-system#10174 

### Description

The Cloud masthead (and other mastheads) simple menus were not closing upon hitting Esc if focus was inside the submenu. This PR fixes that. 

### Changelog

**Changed**

- Adds ability to close Cloud Masthead non-megamenu (and all non-megamenus) with Esc key, just as megamenus already do.

Co-authored-by: Pauline <pauline.judge@lullabot.com>
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@annawen1 annawen1 annawen1 approved these changes

@jkaeser jkaeser jkaeser approved these changes

@kennylam kennylam Awaiting requested review from kennylam kennylam is a code owner automatically assigned from carbon-design-system/carbon-for-ibm-com-members

@emyarod emyarod Awaiting requested review from emyarod emyarod is a code owner automatically assigned from carbon-design-system/carbon-for-ibm-com-members

Assignees
No one assigned
Labels
feature flag package: carbon web components package: web components Work necessary for the IBM.com Library web components package Ready to merge Label for the pull requests that are ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Cloud-Masthead v1]: a11y Feature/improvement - support Esc key to close the Dropdown in masthead component
5 participants
@pjudge @ibmdotcom-bot @jkaeser @kennylam @annawen1