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: Update prop names to match aria labels #551

Merged
merged 12 commits into from
Apr 17, 2020
Merged

Fix: Update prop names to match aria labels #551

merged 12 commits into from
Apr 17, 2020

Conversation

mannycarrera4
Copy link
Contributor

@mannycarrera4 mannycarrera4 commented Apr 7, 2020

Summary

Fixes: #526

Changes:

DrawerHeader

The following props where renamed for appropriate aria naming and clarity •

  • closeIconLabel -> closeIconAriaLabel

SearchBar

The following props where renamed for appropriate aria naming and clarity

  • submitLabel -> submitAriaLabel
  • openButtonLabel -> openButtonAriaLabel
  • closeButtonLabel -> closeButtonAriaLabel

Menu

The following props where renamed for appropriate aria naming and clarity

  • labeledBy -> 'aria-labelledby'

Popup

The following props where renamed for appropriate aria naming and clarity

  • closeLabel -> closeButtonAriaLabel

The property title was removed since we already had an aria-label attached to the close button


SidePanel

The following props where renamed for appropriate aria naming and clarity

  • closeNavigationLabel -> closeNavigationAriaLabel
  • openNavigationLabel -> openNavigationAriaLabel

Skeleton

The following props where renamed for appropriate aria naming and clarity

  • loadingLabel -> 'aria-label

@mannycarrera4 mannycarrera4 changed the base branch from master to prerelease/v4 April 7, 2020 21:45
@cypress
Copy link

cypress bot commented Apr 7, 2020



Test summary

219 0 1 0


Run details

Project canvas-kit
Status Passed
Commit b4c6d29 ℹ️
Started Apr 17, 2020 8:34 PM
Ended Apr 17, 2020 8:37 PM
Duration 02:15 💡
OS Linux Ubuntu Linux - 18.04
Browser Electron 80

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@mannycarrera4
Copy link
Contributor Author

So some of my findings:
• Sometimes we have a prop for aria labels
• Sometimes those props are used for other properties such as titles
• Sometimes we bake in the aria labels and they're not props
• In icon button, we have an aria label prop, but it's overridden by the aria-label attribute, look at story for example vs the component

@mannycarrera4
Copy link
Contributor Author

mannycarrera4 commented Apr 13, 2020

Prop used for aria attributes and other props
We have a few components that use a prop for aria attributes and other properties such as title.

Question
Should we add a different prop solely for the aria attribute or just document that is used for multiple reasons.

Example:
In Popup.tsx we have a closeLabel prop that is used for the aria label AND the title attribute.

The same goes for ColorPicker.tsx. The prop customHexInputLabel is used for the label and the aria label for for the check button.

Exposing Aria attributes for customization

In Drawer.tsx and Menu.tsx we expose existing aria attributes to allow for customization. Do we always expose these in the string literal format such as aria-labeledBy or camel cased?

Proposal:
• If we expose an aria attribute for flexibility, we specifically say that it's for aria purposes, closeLabel -> closeAriaLabel
• If the prop is used for multiple things, we break it up into separate props for clarification
• If the props is exposed in the API but not explicitly used such as in Drawer, we leave as is in string literal format

@anicholls
Copy link
Contributor

@mannycarrera4 Thanks for pulling together your proposal.

I'm a bit confused by

If we expose an aria attribute for flexibility, we specifically say that it's for aria purposes, closeLabel -> closeAriaLabel

vs.

If the props is exposed in the API but not explicitly used for anything other than aria-label, we leave as is in string literal format

Are these not two different approaches to the same case?

@mannycarrera4
Copy link
Contributor Author

@anicholls I was looking at two different use cases and was a little confused, mostly around the example of Drawer.tsx vs Popup.tsx in popup we add it to the api, but it is passed through on the attribute aria-label and in Drawer we don't pass that prop directly into the corresponding aria attribute, which is a little confusing but allows the consumer the flexibility

@mannycarrera4 mannycarrera4 marked this pull request as ready for review April 15, 2020 18:35
Copy link
Contributor

@anicholls anicholls left a comment

Choose a reason for hiding this comment

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

This is looking good 👍 Just a few minor thoughts

@NicholasBoll NicholasBoll added the breaking-change A change that will break something for consumers label Apr 16, 2020
Copy link
Contributor

@anicholls anicholls left a comment

Choose a reason for hiding this comment

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

@mannycarrera4 Can you document the breaking changes in the PR description before merging?

@anicholls anicholls added the 4.x label Apr 17, 2020
Co-Authored-By: Nicholas Boll <nicholas.boll@gmail.com>
@NicholasBoll NicholasBoll merged commit dcefc94 into Workday:prerelease/v4 Apr 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.x breaking-change A change that will break something for consumers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename label props to the appropriate aria label
3 participants