-
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(StructureList): refactor class components, add aria table roles #8084
feat(StructureList): refactor class components, add aria table roles #8084
Conversation
@tw15egan can't figure out why the unit test for unique IDs is failing. Don't have a whole lot of enzyme experience but is that failing test looking for a |
Deploy preview for carbon-elements ready! Built with commit 686815d |
Deploy preview for carbon-components-react ready! Built with commit 70eba6e https://deploy-preview-8084--carbon-components-react.netlify.app |
Deploy preview for carbon-components-react ready! Built with commit 686815d https://deploy-preview-8084--carbon-components-react.netlify.app |
const { children, className, ...other } = props; | ||
const classes = classNames(`${prefix}--structured-list-tbody`, className); | ||
const { labelRows, setLabelRows } = useState(null); | ||
const { rowSelected, setRowSelected } = useState(0); |
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.
this is kinda weird, probably something i'm missing, but setLabel
and rowSelected
were tracked in state, but unused?
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.
AFAIK StructuredList
hasn't been touched in quite some time and could definitely have some leftover unused hooks that may need to be refactored
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.
Totally the case -- just got rid of em
@dakahn I think we need to update tests! |
|
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.
yay!
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 👍 ✅
@tw15egan yep! This refactor is preliminary work before we start hacking on 1937 👍🏽 |
part of #1937
We're working on #1937 and taking the opportunity to update StructuredList from an es6 class components to functional components w/hooks. StructuredList also seemed to to have no table markup whatsoever so i've added some basic aria roles making the component identifiable as a table to screen readers.
Testing / Reviewing
This PR shouldn't have any visible changes or functionality changes whatsoever.