-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
feat(DataTable): allow additional contents to expand cells #5060
feat(DataTable): allow additional contents to expand cells #5060
Conversation
This change adds `children` prop support to `<TableExpandHeader>`. This provides applications with more control so application can better describe the expand cell. Fixes carbon-design-system#5028.
Deploy preview for the-carbon-components ready! Built with commit f8a2093 https://deploy-preview-5060--the-carbon-components.netlify.com |
Deploy preview for carbon-components-react ready! Built with commit f8a2093 https://deploy-preview-5060--carbon-components-react.netlify.com |
Deploy preview for carbon-elements ready! Built with commit f8a2093 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to add a prop type for children
? If so, should it be PropTypes.string
or PropTypes.node
? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me, I'm leaning towards setting the children
prop type to string
based on current usage examples
If they are going to need to hide whatever text they place there for a11y purposes, they're going to have to pass in a node with a |
If we're adding this for a11y should we wrap |
@joshblack depends on if this is the only use case we have for adding this text. If yes, then yeah we might as well hook it all up for them. Not sure if users will ever need this content to be visible |
Totally fair! Real quick, when looking at the original issue are they looking for the |
Quick look at the original issue I couldn't find any mention of |
I think it's just me misunderstanding things, thanks for checking in! It does seem like we'd want to go down the route of giving the option to supply text to be announced by a screen reader but not necessarily have it visually rendered, would that make sense with how everyone is interpreting this with respect to the data table design? |
Looking at #5028, the hard requirement seems the ability for application to add text to table expand cells. It makes sense for the sake of flexibility. The specifics above may have some room for discussion, so what #5028 requests for sounds a good stop-gap. |
This change adds
children
prop support to<TableExpandHeader>
. This provides applications with more control so application can better describe the expand cell.Fixes #5028.
Changelog
New
children
prop support to<TableExpandHeader>
.Testing / Reviewing
Testing should make sure our data table is not broken.