-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
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
[TableCell] Add "scope" attribute for th #10277
[TableCell] Add "scope" attribute for th #10277
Conversation
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it should.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, good catch with the wrapper.setProps({ variant: 'head' });
below. We try not to follow this pattern with the other tests.
@Z-AX It's a great first pull request 👌🏻. Thank you for giving it a shot! |
Fixes #10269.