-
-
Notifications
You must be signed in to change notification settings - Fork 31.6k
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
Merged
oliviertassinari
merged 3 commits into
mui:v1-beta
from
dmythro:table-cell-head-scope-attr
Feb 14, 2018
Merged
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -76,6 +76,13 @@ describe('<TableCell />', () => { | |
assert.strictEqual(wrapper.name(), 'th'); | ||
assert.strictEqual(wrapper.hasClass(classes.root), true); | ||
assert.strictEqual(wrapper.hasClass(classes.typeHead), true, 'should have the head class'); | ||
assert.strictEqual(wrapper.props().scope, 'col', 'should have the correct scope attribute'); | ||
}); | ||
|
||
it('should render specified scope attribute even when in the context of a table head', () => { | ||
const wrapper = shallow(<TableCell scope="row" />); | ||
wrapper.setContext({ table: { head: true } }); | ||
assert.strictEqual(wrapper.props().scope, 'row', 'should have the specified scope attribute'); | ||
}); | ||
|
||
it('should render a th with the footer class when in the context of a table footer', () => { | ||
|
@@ -104,6 +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, undefined, 'should have the correct scope attribute'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
}); | ||
|
||
it('should render without head class when variant is body, overriding context', () => { | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 forclasses.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 thescope
.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:
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.
variant="head"
is purely UI specific, it won't change the DOM structure with theth
vstd
elements. For instance, I'm usingtd
+variant="head"
in some cases. So I disagree. I think that it's taking too much assumption.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 rendertable / thead / tr / th (scope="col") Some TH Text
?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 :)