[TableCell] Add "scope" attribute for th #10277
Conversation
oliviertassinari
added
new feature
|
@Z-AX I had fixed the TypeScript build. Let me know if the pull-request still address your need. |
src/Table/TableCell.js
Outdated
| @@ -72,14 +72,18 @@ function TableCell(props, context) { | |||
| Component = table && table.head ? 'th' : 'td'; | |||
| } | |||
|
|
|||
| const isTypeHead = variant ? variant === 'head' : table && table.head; | |||
| let scope = scopeProp; | |||
| if (!scope && table && table.head) { | |||
There was a problem hiding this comment.
Don't think it is correct. Should work for variant === 'head' if set, that's why I had a constant for the same check as for classes.typeHead className.
There was a problem hiding this comment.
That's why you actually had to modify the test as well, but that test was to actually check that head table cells have the scope.
There was a problem hiding this comment.
Looking now I'd say it should look like this:
let scope = scopeProp;
if (!scope && (variant ? variant === 'head' : table && table.head)) {
scope = 'col';
}
If variant is specified follow it's preference, otherwise check if table.head.
There was a problem hiding this comment.
variant="head" is purely UI specific, it won't change the DOM structure with the th vs td elements. For instance, I'm using td + variant="head" in some cases. So I disagree. I think that it's taking too much assumption.
There was a problem hiding this comment.
Yeah, makes sense to me now.
So, am I correct that table && table.head will fire for structure like this and wii render table / thead / tr / th (scope="col") Some TH Text?
<TableHead>
<TableRow>
<TableCell>
Some TH Text
</TableCell>
</TableRow>
</TableHead>There was a problem hiding this comment.
Then I don’t have anything else, let’s merge it :)
src/Table/TableCell.spec.js
Outdated
| @@ -112,7 +111,7 @@ describe('<TableCell />', () => { | |||
| wrapper.setContext({ table: { footer: true } }); | |||
| wrapper.setProps({ variant: 'head' }); | |||
| assert.strictEqual(wrapper.hasClass(classes.typeHead), true); | |||
| assert.strictEqual(wrapper.props().scope, 'col', 'should have the correct scope attribute'); | |||
| assert.strictEqual(wrapper.props().scope, undefined, 'should have the correct scope attribute'); | |||
There was a problem hiding this comment.
The props().scope should be 'col' for <TableCell variant="head" />.
src/Table/TableCell.spec.js
Outdated
| wrapper.setContext({ table: { head: true } }); | ||
| wrapper.setProps({ scope: 'row' }); |
There was a problem hiding this comment.
That is a weird refactoring :) I was actually trying to follow code styles as you have in this file as well (like wrapper.setProps({ variant: 'head' }); below).
There was a problem hiding this comment.
Oh, good catch with the wrapper.setProps({ variant: 'head' }); below. We try not to follow this pattern with the other tests.
oliviertassinari
changed the title
oliviertassinari
added
accessibility
|
@Z-AX It's a great first pull request 👌🏻. Thank you for giving it a shot! |
Fixes #10269.