Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[TableCell] Add "scope" attribute for th #10277

Merged
merged 3 commits into from
Feb 14, 2018
Merged

[TableCell] Add "scope" attribute for th #10277

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
730e54b
[TableCell] No "scope" attribute added for type "head"
dmythro Feb 13, 2018
f919337
fix typescript
oliviertassinari Feb 13, 2018
49b04a6
clean
oliviertassinari Feb 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions pages/api/table-cell.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ filename: /src/Table/TableCell.js
| component | union:&nbsp;string&nbsp;&#124;<br>&nbsp;func<br> | | The component used for the root node. Either a string to use a DOM element or a component. |
| numeric | bool | false | If `true`, content will align to the right. |
| padding | enum:&nbsp;'default'&nbsp;&#124;<br>&nbsp;'checkbox'&nbsp;&#124;<br>&nbsp;'dense'&nbsp;&#124;<br>&nbsp;'none'<br> | 'default' | Sets the padding applied to the cell. |
| scope | string | | Set scope attribute. |
| sortDirection | enum:&nbsp;'asc'&nbsp;&#124;<br>&nbsp;'desc'&nbsp;&#124;<br>&nbsp;false<br> | | Set aria-sort direction. |
| variant | enum:&nbsp;'head'&nbsp;&#124;<br>&nbsp;'body'&nbsp;&#124;<br>&nbsp;'footer'<br> | | Specify the cell type. By default, the TableHead, TableBody or TableFooter parent component set the value. |

Expand Down
3 changes: 0 additions & 3 deletions src/Table/TableCell.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ export interface TableCellProps extends StandardProps<TableCellBaseProps, TableC
component?: React.ReactType<TableCellBaseProps>;
numeric?: boolean;
padding?: Padding;
scope?: Scope;
sortDirection?: SortDirection;
type?: Type;
}
Expand All @@ -23,8 +22,6 @@ export type TableCellBaseProps = React.ThHTMLAttributes<HTMLTableHeaderCellEleme

export type Padding = 'default' | 'checkbox' | 'dense' | 'none';

export type Scope = 'col' | 'row' | false;

export type SortDirection = 'asc' | 'desc' | false;

export type Type = 'head' | 'body' | 'footer';
Expand Down
19 changes: 9 additions & 10 deletions src/Table/TableCell.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ function TableCell(props, context) {
sortDirection,
numeric,
padding,
scope,
scope: scopeProp,
variant,
...other
} = props;
Expand All @@ -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) {
Copy link
Author

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.

Copy link
Author

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.

Copy link
Author

@dmythro dmythro Feb 14, 2018 •
edited

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.

Copy link
Member

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 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.

capture d ecran 2018-02-14 a 13 52 07

Copy link
Author

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>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it should.

Copy link
Author

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 :)

scope = 'col';
}

const className = classNames(
classes.root,
{
[classes.numeric]: numeric,
[classes[`padding${capitalize(padding)}`]]: padding !== 'none' && padding !== 'default',
[classes.paddingDefault]: padding !== 'none',
[classes.typeHead]: isTypeHead,
[classes.typeHead]: variant ? variant === 'head' : table && table.head,
[classes.typeBody]: variant ? variant === 'body' : table && table.body,
[classes.typeFooter]: variant ? variant === 'footer' : table && table.footer,
},
Expand All @@ -91,13 +95,8 @@ function TableCell(props, context) {
ariaSort = sortDirection === 'asc' ? 'ascending' : 'descending';
}

let cellScope = scope;
if (!scope && isTypeHead) {
cellScope = 'col';
}

return (
<Component className={className} aria-sort={ariaSort} scope={cellScope} {...other}>
<Component className={className} aria-sort={ariaSort} scope={scope} {...other}>
{children}
</Component>
);
Expand Down Expand Up @@ -132,7 +131,7 @@ TableCell.propTypes = {
/**
* Set scope attribute.
*/
scope: PropTypes.oneOf(['col', 'row', false]),
scope: PropTypes.string,
/**
* Set aria-sort direction.
*/
Expand Down
5 changes: 2 additions & 3 deletions src/Table/TableCell.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,8 @@ describe('<TableCell />', () => {
});

it('should render specified scope attribute even when in the context of a table head', () => {
const wrapper = shallow(<TableCell />);
const wrapper = shallow(<TableCell scope="row" />);
wrapper.setContext({ table: { head: true } });
wrapper.setProps({ scope: 'row' });
Copy link
Author

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).

Copy link
Member

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.

assert.strictEqual(wrapper.props().scope, 'row', 'should have the specified scope attribute');
});

Expand Down Expand Up @@ -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');
Copy link
Author

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" />.

});

it('should render without head class when variant is body, overriding context', () => {
Expand Down