Skip to content

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

Merged
abbeyhrt merged 12 commits into
carbon-design-system:mainfrom
abbeyhrt:feat/default-props-change
Oct 30, 2021
Merged

fix(carbon-react): removes default props for accessible placeholders#9741
abbeyhrt merged 12 commits into
carbon-design-system:mainfrom
abbeyhrt:feat/default-props-change

Conversation

@abbeyhrt

Copy link
Copy Markdown
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 dakahn September 24, 2021 18:05
@abbeyhrt abbeyhrt requested a review from a team as a code owner September 24, 2021 18:05
@abbeyhrt abbeyhrt requested a review from aledavila September 24, 2021 18:05
@netlify

netlify Bot commented Sep 24, 2021

Copy link
Copy Markdown

✔️ 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

netlify Bot commented Sep 24, 2021

Copy link
Copy Markdown

✔️ 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

netlify Bot commented Sep 24, 2021

Copy link
Copy Markdown

✔️ 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

@joshblack joshblack left a comment

Copy link
Copy Markdown
Contributor

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 👀

Comment thread packages/react/src/components/FileUploader/FileUploader.js Outdated
Comment thread packages/react/src/components/NumberInput/NumberInput.js Outdated
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

abbeyhrt commented Oct 4, 2021

Copy link
Copy Markdown
Contributor Author

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
Comment thread packages/react/src/components/Tab/Tab.js Outdated
Comment on lines +48 to +50
id: FeatureFlags.enabled('enable-v11-release')
? PropTypes.string.isRequired
: PropTypes.string,

Copy link
Copy Markdown
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?

Comment thread packages/react/src/components/SelectItemGroup/SelectItemGroup.js Outdated
Comment thread packages/react/src/components/NumberInput/NumberInput.js
Comment thread packages/react/src/components/NumberInput/NumberInput.js Outdated
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