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

refactor(OverflowMenu): ariaLabel to aria-label #13256

Closed
wants to merge 6 commits into from
Closed

refactor(OverflowMenu): ariaLabel to aria-label #13256

wants to merge 6 commits into from

Conversation

jsehull
Copy link
Contributor

@jsehull jsehull commented Feb 28, 2023

#12802

Update OverflowMenu to use the proper aria-label

Changelog

New

  • n/a

Changed

  1. added deprecated() to the old propType
  2. added new usage for aria-label
  3. kept backwards compatibility so that both aria versions can work with ariaLabel (deprecated) and aria-label (new).

Removed

  • n/a

Testing / Reviewing

  1. Open Storybook > OverflowMenu in browser for Dev Tool testing
  2. Open OverflowMenu.stories.js in code
  3. Add one aria label type at a time to the component in story code.
    1. Only one version can be active at a time.
    • aria-label (new)
    • ariaLabel (deprecated))
    1. Be sure to find the correct usage for the given component as explained in code or on the story itself
    1. Test examples below.
    • Note: For my testing, I duplicate the lines and then comment out the one I am not using. The image blow does NOT yet comment out one of the test lines:
      image
  1. confirm that both propTypes still display in Dev Tools as intended
  2. confirm that Storybook > Docs > Component API contains both propTypes as options

@jsehull jsehull requested a review from a team as a code owner February 28, 2023 16:05
@netlify
Copy link

netlify bot commented Feb 28, 2023

Deploy Preview for carbon-components-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 89c7304
🔍 Latest deploy log https://app.netlify.com/sites/carbon-components-react/deploys/63ffbb7f49779200072108a5
😎 Deploy Preview https://deploy-preview-13256--carbon-components-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 settings.

@jsehull
Copy link
Contributor Author

jsehull commented Feb 28, 2023

I'm going to work on getting the yarn test -u snapshots updated here

@jsehull
Copy link
Contributor Author

jsehull commented Feb 28, 2023

Hey team, would you mind looking at the above "Resolve Conflicts" situation for me? It really seems as though the entire OverflowMenu was NOT included, and now it is. I'm not sure how to verify snapshot accuracy (just to run the yarn command).

Would this be safe to resolve keeping the old and new?
@francinelucca @andreancardona

@francinelucca
Copy link
Collaborator

Hey team, would you mind looking at the above "Resolve Conflicts" situation for me? It really seems as though the entire OverflowMenu was NOT included, and now it is. I'm not sure how to verify snapshot accuracy (just to run the yarn command).

Would this be safe to resolve keeping the old and new? @francinelucca @andreancardona

I think this is RTL migration related, when we used enzyme the snapshots generated were related to the JSX and with RTL they're of the resulting HTML (from what I can tell) https://github.com/carbon-design-system/carbon/pull/13172/files#diff-9fd34dc5f64cec8d8dc7b13413afd7d71c292776b551948a3ff018f7839afd82.
I just think Jesse's branch is not updated yet (has the OverflowMenu stuff, which is no longer on main since the component name wouldn't show up in the HTML as a tag) so it's causing those conflicts

@jsehull
Copy link
Contributor Author

jsehull commented Feb 28, 2023

Resolution suggestions? Do I need to sync my fork and push a new a branch here? Any yarn commands to use?

@tay1orjones
Copy link
Member

tay1orjones commented Mar 1, 2023

@jsehull I resolved the conflicts and pushed up the commit

What I did was:

  • git pull upstream main --no-rebase
  • yarn && yarn build && yarn test -u
  • Commit and push up changes

I also noticed some tests are failing and fixed a few of them. There might be more usages of ariaLabel within tests that need to be updated to ariaLabel.

@netlify
Copy link

netlify bot commented Mar 1, 2023

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit 6197557
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/63ff918973dcd400085b6846
😎 Deploy Preview https://deploy-preview-13256--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 settings.

@jsehull
Copy link
Contributor Author

jsehull commented Mar 1, 2023

@tay1orjones Thank you for the process steps, I'll sort this out for the rest of the components involved

@jsehull jsehull requested review from a team and laurenmrice as code owners March 1, 2023 20:54
@jsehull
Copy link
Contributor Author

jsehull commented Mar 1, 2023

Real talk Team - I spilled coffee all over git and this thing may need to be put out to pasture and redone 😅

@tay1orjones this may be a FALSE POSITIVE. I know my changes worked, but I dragged in other changes we don't want. This should NOT be merged AS-IS.

@tw15egan
Copy link
Collaborator

tw15egan commented Mar 2, 2023

@jsehull do you want me to close this and resubmit?

@jsehull
Copy link
Contributor Author

jsehull commented Mar 2, 2023

@tw15egan Yes please. I'll create a new PR

@tw15egan tw15egan closed this Mar 2, 2023
@jsehull jsehull deleted the 12802/overflowmenu branch March 2, 2023 17:01
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.

4 participants