fix(table-batch-action): remove default iconDescription, add proptype check, update stories #3928
Conversation
|
Deploy preview for the-carbon-components ready! Built with commit 61634e4 https://deploy-preview-3928--the-carbon-components.netlify.com |
|
Deploy preview for carbon-elements ready! Built with commit 61634e4 |
|
Deploy preview for carbon-components-react ready! Built with commit 61634e4 https://deploy-preview-3928--carbon-components-react.netlify.com |
|
Deploy preview for the-carbon-components ready! Built with commit b3fa472 https://deploy-preview-3928--the-carbon-components.netlify.com |
|
Deploy preview for carbon-components-react ready! Built with commit b3fa472 https://deploy-preview-3928--carbon-components-react.netlify.com |
|
Deploy preview for carbon-elements ready! Built with commit b3fa472 |
| */ | ||
| iconDescription: PropTypes.string.isRequired, | ||
| hasIconOnly: PropTypes.bool, |
There was a problem hiding this comment.
Is hasIconOnly being used anywhere?
There was a problem hiding this comment.
Oh right, it's not 😅
I could update the batch actions story to include sample usage of hasIconOnly with iconDescription? Something like this, maybe for that last button in the batch actions series ("Download"):
<TableBatchAction
hasIconOnly
renderIcon={Download}
iconDescription="Download the selected rows"
onClick={batchActionClick(selectedRows)}
/>Or would you prefer to remove hasIconOnly from this?
Closes #3809
The 2 big takeaways for me from #3809 are:
iconDescriptionis not necessary (and shouldn't be present)iconDescriptionshould not be "Add" by default because that adds noise for screen readersBoth of these points are very similar to what's been changed with the
Button's props recently (https://github.com/carbon-design-system/carbon/blob/master/packages/react/src/components/Button/Button.js#L177) , so I used that component as a guide here.Changelog
New
hasIconOnlybutton prop inTableBatchAction(referring to same prop inButton: https://github.com/carbon-design-system/carbon/blob/master/packages/react/src/components/Button/Button.js#L189)Changed
iconDescriptionis no longer requirediconDescriptionprop type to add similar warning as the one that already exists inButton(https://github.com/carbon-design-system/carbon/blob/master/packages/react/src/components/Button/Button.js#L177)Removed
iconDescription(same as inButton- https://github.com/carbon-design-system/carbon/blob/master/packages/react/src/components/Button/Button.js#L204)iconDescriptionvalue because button text is already provided in all of those examplesTesting / Reviewing
Please note that I am unable to test in JAWS myself (cc @dakahn how should we verify this? thanks for your help!)