-
Notifications
You must be signed in to change notification settings - Fork 933
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
Feature/issue 871 (setCellHeaderProps) and Feature/issue 714 (setTableProps) #889
Changes from 7 commits
8484f12
b763cc5
4d2a4c9
fe8ccf6
47afa1a
57b97a3
90f37b8
ceee9fe
2b1b4ac
3b99949
a9ee3a3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,18 +12,26 @@ const defaultBodyCellStyles = theme => ({ | |
[theme.breakpoints.down('sm')]: { | ||
display: 'inline-block', | ||
fontSize: '16px', | ||
height: '24px', | ||
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. Could we hold off on the stylistic changes here or put in another PR? I believe there were some modifications here since you opened this that might have solved some of the style issues, but also, if we keep them separate, then we can discuss them separately and then they won't hold up the other work here. 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. They're tied to the functionality a setTableProps method would add (ie, "dense table"). Without these changes, applying a dense table breaks stacked mode. There's actually bugs in the existing code, if you view the Customize Styling example you'll see one of them: https://codesandbox.io/s/github/gregnb/mui-datatables/ The CSS solution I wound up with is simple. Initially I tried to be fancy, but it wasn't needed and didn't cover certain edge cases (basically a height parameter is needed for causes when cell values are empty or just a space). 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. Right, so the problem is just that the PR is a two-for-one deal. In the future, I'd prefer to have them kept separate as this makes reviewing easier, but it's also much easier to trace back any future issues or regressions to a specific commit (since we squash merges) and enables us to rollback specific sets of functionality without losing others that work well, if need be. |
||
width: 'calc(50% - 80px)', | ||
width: '50%', | ||
whiteSpace: 'nowrap', | ||
boxSizing: 'border-box', | ||
height: '32px', | ||
'&:nth-last-child(2)': { | ||
borderBottom: 'none' | ||
}, | ||
}, | ||
}, | ||
responsiveStacked: { | ||
[theme.breakpoints.down('sm')]: { | ||
display: 'inline-block', | ||
fontSize: '16px', | ||
width: 'calc(50% - 80px)', | ||
width: '50%', | ||
whiteSpace: 'nowrap', | ||
height: '24px', | ||
boxSizing: 'border-box', | ||
height: '32px', | ||
'&:last-child': { | ||
borderBottom: 'none' | ||
}, | ||
}, | ||
}, | ||
}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -89,7 +89,8 @@ class TableHeadCell extends React.Component { | |
|
||
render() { | ||
const { isSortTooltipOpen, isHintTooltipOpen } = this.state; | ||
const { children, classes, options, sortDirection, sort, hint, print } = this.props; | ||
const { children, classes, options, sortDirection, sort, hint, print, cellHeaderProps } = this.props; | ||
const { className, ...otherProps } = cellHeaderProps; | ||
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. Similar to the other comment about checking for the 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. I'm not sure I follow. Check that it's set to a string? |
||
const sortActive = sortDirection !== 'none' && sortDirection !== undefined ? true : false; | ||
const ariaSortDirection = sortDirection === 'none' ? false : sortDirection; | ||
|
||
|
@@ -100,14 +101,17 @@ class TableHeadCell extends React.Component { | |
...(ariaSortDirection ? { direction: sortDirection } : {}), | ||
}; | ||
|
||
const cellClass = classNames({ | ||
[classes.root]: true, | ||
[classes.fixedHeader]: options.fixedHeader, | ||
'datatables-noprint': !print, | ||
}); | ||
const cellClass = classNames( | ||
{ | ||
[classes.root]: true, | ||
[classes.fixedHeader]: options.fixedHeader, | ||
'datatables-noprint': !print, | ||
}, | ||
className, | ||
); | ||
|
||
return ( | ||
<TableCell className={cellClass} scope={'col'} sortDirection={ariaSortDirection}> | ||
<TableCell className={cellClass} scope={'col'} sortDirection={ariaSortDirection} {...otherProps}> | ||
{options.sort && sort ? ( | ||
<Tooltip | ||
title={options.textLabels.body.toolTip} | ||
|
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.
Maybe we should be more defensive here. In case the function doesn't return an object, it will error out below. What do you think?
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.
I don't know, that's not how the existing setCellProps and setRowProps functions work. Trying out other return values in the console, it doesn't look like anything breaks with the spread operator, they just wouldn't be setting any fields if they had the function return a non-object (with the exception of a "string" type, where they'd set numerical fields on the component).
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 others don't work this way because they aren't checking an object for a specific property. The new functions you added are checking to see if
classNames
appears on the object, and this will error for non-objects, where the others won't, with the exception of strings.