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-v2): fix masthead-contact & add DDO for testing #9359

Merged
merged 7 commits into from
Sep 14, 2022
Merged

fix(masthead-v2): fix masthead-contact & add DDO for testing #9359

merged 7 commits into from
Sep 14, 2022

Conversation

proeung
Copy link
Contributor

@proeung proeung commented Sep 12, 2022 •
edited
Loading

Related Ticket(s)

#9360

Description

  • This PR fixes the issue where the "Contact" CTA is not initializing the contact module's Let's Talk module when the has-contact attribute is set to true.

Changelog

Changed

  • Update the Masthead component CodeSandbox to include a test DDO and cm-app.min.js script so that we can verify that the behavior is working as expected.
Screen.Recording.2022-09-12.at.11.35.02.AM.mov

Testing Instructions

@proeung proeung added package: web components Work necessary for the IBM.com Library web components package owner: Innovation Team used when the engineering work will be done by Hybrid Cloud with DDS engineers as consultants test: e2e used for automated e2e testing labels Sep 12, 2022
@proeung proeung requested a review from a team as a code owner September 12, 2022 15:42
@proeung proeung requested review from RichKummer, ariellalgilmore, jkaeser, andy-blum and jwitkowski79 and removed request for a team September 12, 2022 15:42
@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Sep 12, 2022 •
edited
Loading

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

Built with commit: 7ba09982df9ef714dac59f000b9b7c415158547a

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Sep 12, 2022 •
edited
Loading

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

Built with commit: 7ba09982df9ef714dac59f000b9b7c415158547a

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Sep 12, 2022 •
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/9359/index.html

Built with commit: 7ba09982df9ef714dac59f000b9b7c415158547a

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Sep 12, 2022 •
edited
Loading

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/9359/index.html

Built with commit: 7ba09982df9ef714dac59f000b9b7c415158547a

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Sep 12, 2022 •
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/9359/index.html

Built with commit: 7ba09982df9ef714dac59f000b9b7c415158547a

jkaeser
jkaeser reviewed Sep 12, 2022
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 good from a code perspective. Waiting for the CodeSandbox example to do functional testing.

@proeung proeung requested a review from annawen1 September 12, 2022 20:47
@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Sep 12, 2022 •
edited
Loading

Deploy preview created for package "Web Components (Codesandbox Examples)":
https://webcomponents-codesandbox.s3-web.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/9359/index.html

Built with commit: 7ba09982df9ef714dac59f000b9b7c415158547a

@proeung
Copy link
Contributor Author

proeung commented Sep 12, 2022

CodeSandbox example deploy preview is ready for testing - https://webcomponents-codesandbox.s3-web.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/9359/masthead/index.html

cc: @jwitkowski79 @oliviaflory

jwitkowski79
jwitkowski79 approved these changes Sep 13, 2022
View reviewed changes
Copy link

@jwitkowski79 jwitkowski79 left a comment

Choose a reason for hiding this comment

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

@proeung This looks good and is working as expected. Would love if @oliviaflory could also take a peek at it

@proeung proeung requested a review from oliviaflory September 13, 2022 19:41
@proeung proeung mentioned this pull request Sep 13, 2022
Merged
@oliviaflory
Copy link
Contributor

@jkaeser @proeung mostly there!

I like that when you have a mega menu open and then select the contact us button, the mega menu collapses/closed.
I think the reverse should also be true. When I select the Contact us button first, the contact module opens, and if I then select a mega menu the chat module stays open and obstructs part of the mega menu content

contact module mega menu Sep-13-2022 17-49-10

Questions that we should run by the accessibility team:

  1. Should clicking the chat button again (after the chat module is opened) trigger the chat module to close? Asking because otherwise the user has to move their mouse to the contact module to close, and all other navigation items (mega menus and profile menu) close the menus on a second click.
  2. Should the focus state move to the Chat module once the user clicks it for keyboard users?
    • When testing with the screen reader, there is no announcement that the chat module has opened
    • After you select the Contact us button, a keyboard user has to tab through the entire page before getting to the contact module. This doesn't seem like a great experience (hence the suggestion to move the focus state). Shift tab would go back to the contact us button.

@proeung proeung mentioned this pull request Sep 14, 2022
Closed
@proeung
Copy link
Contributor Author

proeung commented Sep 14, 2022

@oliviaflory Thanks for your review! What you've outlined makes sense and is great for usability, however, it's out of scope from this issue (#9360) and one that I'm not sure if it's feasible for Phase 1.

For this early Oct launch, we're just trying to prioritize some of the big line item bugs seen not just within the components, but other bugs that arise in the platform integrations (eg. AEM, Drupal, etc) and this one will most likely go into Phase 2 work. I've created a new ticket to capture your feedback along with the accessibility concerns, which we can revisit for Phase 2. Also, as far as I'm aware the global masthead will be launched without the contact button enabled.

GH issue: #9369
JIRA ticket: https://jsw.ibm.com/browse/ADCMS-2602

@proeung proeung added the Ready to merge Label for the pull requests that are ready to merge label Sep 14, 2022
@proeung proeung mentioned this pull request Sep 14, 2022
Closed
@kodiakhq kodiakhq bot merged commit 2c924be into carbon-design-system:feat/masthead-v2 Sep 14, 2022
jkaeser pushed a commit to jkaeser/carbon-for-ibm-dotcom that referenced this pull request Apr 6, 2023
@proeung @jkaeser
fix(masthead-v2): fix masthead-contact & add DDO for testing (carbon-…
09b8f12 
…design-system#9359)

### Related Ticket(s)

carbon-design-system#9360

### Description

- This PR fixes the issue where the "Contact" CTA is not initializing the contact module's Let's Talk module when the `has-contact` attribute is set to `true`.

### Changelog

**Changed**

- Update the `Masthead` component CodeSandbox to include a test `DDO` and `cm-app.min.js` script so that we can verify that the behavior is working as expected.


https://user-images.githubusercontent.com/1815714/189696785-e00a7997-7c33-4322-ad6a-008eefacda45.mov

### Testing Instructions
- Visit the `Masthead` codesandbox app - https://webcomponents-codesandbox.s3-web.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/9359/masthead/index.html
- Click on the "Contact" CTA and ensure that the "Let's talk" widget opens.

<!-- React and Web Component deploy previews are enabled by default. -->
<!-- To enable additional available deploy previews, apply the following -->
<!-- labels for the corresponding package: -->
<!-- *** "test: e2e": Codesandbox examples and e2e integration tests -->
<!-- *** "package: services": Services -->
<!-- *** "package: utilities": Utilities -->
<!-- *** "RTL": React / Web Components (RTL) -->
<!-- *** "feature flag": React / Web Components (experimental) -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkaeser jkaeser jkaeser left review comments

@jwitkowski79 jwitkowski79 jwitkowski79 approved these changes

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

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

@andy-blum andy-blum Awaiting requested review from andy-blum

@annawen1 annawen1 Awaiting requested review from annawen1

@oliviaflory oliviaflory Awaiting requested review from oliviaflory

Assignees
No one assigned
Labels
👀 eyes needed owner: Innovation Team used when the engineering work will be done by Hybrid Cloud with DDS engineers as consultants 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 test: e2e used for automated e2e testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@proeung @ibmdotcom-bot @oliviaflory @jwitkowski79 @jkaeser