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(carbon-react): removes default props for accessible placeholders #9741

Merged
merged 12 commits into from
Oct 30, 2021
Merged

fix(carbon-react): removes default props for accessible placeholders #9741

merged 12 commits into from
Oct 30, 2021

Conversation

abbeyhrt
Copy link
Contributor

Closes #9466
(Once #9731 is merged)

Removes default props in v11 for accessible placeholder's and ensures that the props are required.

Also makes Tab's id prop required since the component has an accessibilty violation if it's not provided (reference #9666)

Changelog

Changed

  • iconDescription for FileUploader is now required and default is empty
  • iconDescription's and invalidText's default prop for NumberInput are now empty
  • default prop for label is now empty and label and id are required

Testing / Reviewing

Verify that everything works as expected

@abbeyhrt abbeyhrt requested a review from a team as a code owner September 24, 2021 18:05
@netlify
Copy link

netlify bot commented Sep 24, 2021

✔️ Deploy Preview for carbon-react-next ready!

🔨 Explore the source changes: c693db4

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/617c76a2d401d300089ea623

😎 Browse the preview: https://deploy-preview-9741--carbon-react-next.netlify.app

@netlify
Copy link

netlify bot commented Sep 24, 2021

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: c693db4

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/617c76a2aef7070007bb6429

😎 Browse the preview: https://deploy-preview-9741--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Sep 24, 2021

✔️ Deploy Preview for carbon-components-react ready!
Built without sensitive environment variables

🔨 Explore the source changes: c693db4

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/617c76a2a8fd5700077588e3

😎 Browse the preview: https://deploy-preview-9741--carbon-components-react.netlify.app

Copy link
Contributor

@joshblack joshblack left a comment

Choose a reason for hiding this comment

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

I think for the default props we'll want the value to be undefined versus an empty string.

For dropping isRequired, is it the case that all of these are no longer required? It seemed like we would still want them but want the user to provide them versus the default props being used. Let me know if I'm misunderstanding 👀

packages/react/src/components/FileUploader/FileUploader.js Outdated Show resolved Hide resolved
packages/react/src/components/NumberInput/NumberInput.js Outdated Show resolved Hide resolved
abbeyhrt and others added 2 commits October 4, 2021 11:34
Co-authored-by: Josh Black <josh@josh.black>
Co-authored-by: Josh Black <josh@josh.black>
@abbeyhrt
Copy link
Contributor Author

abbeyhrt commented Oct 4, 2021

For dropping isRequired, is it the case that all of these are no longer required? It seemed like we would still want them but want the user to provide them versus the default props being used. Let me know if I'm misunderstanding 👀

I think you might have it backwards! If v11 is enabled, they are required, if not, they aren't

@dakahn dakahn removed their request for review October 25, 2021 19:30
packages/react/src/components/Tab/Tab.js Outdated Show resolved Hide resolved
Comment on lines 48 to 50
id: FeatureFlags.enabled('enable-v11-release')
? PropTypes.string.isRequired
: PropTypes.string,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we auto-generate the id for the component if it doesn't exist already?

packages/react/src/components/NumberInput/NumberInput.js Outdated Show resolved Hide resolved
abbeyhrt and others added 3 commits October 26, 2021 10:26
Co-authored-by: Josh Black <josh@josh.black>
Co-authored-by: Josh Black <josh@josh.black>
@abbeyhrt abbeyhrt enabled auto-merge (squash) October 27, 2021 15:45
@abbeyhrt abbeyhrt merged commit 51ccb35 into carbon-design-system:main Oct 30, 2021
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.

Do not provide accessible labels by default, for example: "Provide icon description"
3 participants