Feature/issue 871 (setCellHeaderProps) and Feature/issue 714 (setTableProps) #889

patorjk · 2019-08-30T21:36:08Z

Added two new options:

setTableProps (options)
setCellHeaderProps (columns)

They're modeled after the existing setCellProps and setRowProps. The dense table is really only available in Material v4, so the example only removes some padding in Material v3.

Quick view: https://codesandbox.io/s/github/patorjk/mui-datatables/tree/set-props

coveralls · 2019-08-30T21:38:13Z

Coverage increased (+0.009%) to 76.193% when pulling a9ee3a3 on patorjk:set-props into 4f9f2a4 on gregnb:master.

patorjk · 2019-09-11T03:52:58Z

I'm glad this is on your radar. Since discovering setCellProps and setRowsProps I've found them incredibly useful. They provide a lot of power and a nice reprieve from render functions. Some examples:

Using setCellProps I'm able to eliminate 1,700 divs from one of my tables (previously I was wrapping every cell in a div with a style of "white-space:pre").
Using setCellProps and setCellHeaderProps I'm able to have more concise code.
setCellProps and setCellHeaderProps make the issue discussed here: How to set column width and row height? #681 - completely trivial.

Render functions technically allow for the same thing, but I find them dreadful and they're way more complex than would seem. For example, to override a cell header, you really need to add in most of the functionality of TableHeadCell.js, and there's a lot going on in there. I don't think it's reasonable to assume that most devs will take the care to reproduce what's in there, leading to bugs in their code.

Anyway, some time has passed since I put this in. setTableProps may be better off as "tableProps", as there's only 1 table. Additionally, if you decided you like this type of API, you may want to reconsider the searchProps idea of #808 (though that implementation is buggy - it's very close though). searchProps also makes a lot of customization trivial, and itsolves a lot of issues people brought up in #857 (comment).

gabrielliwerant

This one needs an update with latest master, and could we also add some tests for the new prop options?

gabrielliwerant · 2019-09-19T05:24:53Z

src/MUIDataTable.js

@@ -1229,6 +1233,10 @@ class MUIDataTable extends React.Component {
        break;
    }

+    let tableProps = this.options.setTableProps ? this.options.setTableProps() : {};


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?

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

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.

gabrielliwerant · 2019-09-19T05:26:37Z

src/components/TableBodyCell.js

@@ -12,18 +12,26 @@ const defaultBodyCellStyles = theme => ({
    [theme.breakpoints.down('sm')]: {
      display: 'inline-block',
      fontSize: '16px',
-      height: '24px',


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.

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

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.

gabrielliwerant · 2019-09-19T05:31:31Z

src/components/TableHeadCell.js

@@ -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;


Similar to the other comment about checking for the className property, as the dev will get an unhelpful error if they return the wrong type.

I'm not sure I follow. Check that it's set to a string?

gabrielliwerant · 2019-09-19T05:35:26Z

Understood about props vs render, I agree that custom render functions are a bad option when you want to replicate the functionality almost entirely, but with some slight changes. I think it would be a nice option to allow various parts of the table to be exported separately for just this kind of usage, but that's probably a ways away. I think render functions work best when the functionality being added is very custom and much different from what exists, and probably has a bunch of nested components and such. But use cases like those you mention are better served via adding props, so it's nice to have both.

gabrielliwerant · 2019-09-19T05:40:33Z

Hm, I don't see a big difference between setTableProps and tableProps for the API. I think it's better to be consistent with the other options for now. Maybe there's an even better organizing principle that can be used for API naming, like how callbacks all begin with on. I've also been toying with the idea of adding a prefix for boolean options because as of now they are difficult to distinguish from many of the non-boolean options. So this is something I do think about.

Agreed about searchProps having valid use cases outside of the customSearchRender. I'll ping the PR creator in the next day or so and see if he wants to update it. If not, It'll be up for grabs.

patorjk · 2019-09-19T18:29:32Z

I can circle back a little later for tests.

gabrielliwerant · 2019-09-25T03:11:11Z

@patorjk If you want to take on setSearchProps, go for it. I'm going to assume the other dev that submitted the old PR isn't checking up on it, since it's gotten old and he hasn't responded to new messages.

patorjk · 2019-09-25T03:40:44Z

Sure, I'll put something together, I'll loop back to it later this week or next.

…o set-props # Conflicts: # README.md # src/MUIDataTable.js

…/activeIcon-for-search-not-reset-on-toggle * 'master' of github.com:gregnb/mui-datatables: 2.13.0 Prettify files Feature/issue 1036 fixed header (gregnb#1071) Update webpack dev server and related dependencies (gregnb#1069) Feature/issue 871 (setCellHeaderProps) and Feature/issue 714 (setTableProps) (gregnb#889) Feature/issue 937 array and custom update filter support (gregnb#1067) Bugfix/issue 1052 csv download issues (gregnb#1064) Hide toggle button when the row is not expandable (gregnb#1025) add test case to verify reset type (gregnb#1006) Update .gitignore and adding yarn.lock (gregnb#1004) Serverside sorting example (gregnb#986) No Pointer style cursor for table head when no hint or sorting (gregnb#910) Added disableToolbarSelect option (gregnb#874) Add table properties option (gregnb#670)

…eProps) (gregnb#889) * Added setTable props and setCellHeaderProps * Added height back in * Updated to properly work for classNames * Added stacked to initial state * minor adjustments * Fixed edge case of empty cell effecting the display * No getting around edge cases, a height is needed * Allow default in case using the component in isolation (like testing) * Add tests for new props options

patorjk added 4 commits August 30, 2019 16:21

Added setTable props and setCellHeaderProps

8484f12

Added height back in

b763cc5

Updated to properly work for classNames

4d2a4c9

Added stacked to initial state

fe8ccf6

minor adjustments

47afa1a

This was referenced Sep 4, 2019

add className to a column header #894

Open

Varying font size #897

Open

gabrielliwerant added the needs review Useful to mark PRs for what's up next to review label Sep 11, 2019

gabrielliwerant self-requested a review September 11, 2019 03:03

patorjk added 2 commits September 11, 2019 01:09

Fixed edge case of empty cell effecting the display

57b97a3

No getting around edge cases, a height is needed

90f37b8

gabrielliwerant requested changes Sep 19, 2019

View reviewed changes

gabrielliwerant added needs work and removed needs review Useful to mark PRs for what's up next to review labels Sep 19, 2019

Merge branch 'master' into set-props

ceee9fe

gabrielliwerant added 2 commits November 19, 2019 17:18

Allow default in case using the component in isolation (like testing)

2b1b4ac

Add tests for new props options

3b99949

gabrielliwerant added bug enhancement lgtm new feature and removed needs work labels Nov 19, 2019

Merge branch 'master' of https://github.com/gregnb/mui-datatables int…

a9ee3a3

…o set-props # Conflicts: # README.md # src/MUIDataTable.js

gabrielliwerant merged commit 518ed00 into gregnb:master Nov 21, 2019

anthonyraymond mentioned this pull request Nov 26, 2019

Update @types/mui-datatables to new release version DefinitelyTyped/DefinitelyTyped#40646

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/issue 871 (setCellHeaderProps) and Feature/issue 714 (setTableProps) #889

Feature/issue 871 (setCellHeaderProps) and Feature/issue 714 (setTableProps) #889

patorjk commented Aug 30, 2019

coveralls commented Aug 30, 2019 •

edited

Loading

patorjk commented Sep 11, 2019

gabrielliwerant left a comment

gabrielliwerant Sep 19, 2019

patorjk Sep 19, 2019

gabrielliwerant Sep 19, 2019

gabrielliwerant Sep 19, 2019

patorjk Sep 19, 2019

gabrielliwerant Sep 19, 2019

gabrielliwerant Sep 19, 2019

patorjk Sep 19, 2019

gabrielliwerant commented Sep 19, 2019 •

edited

Loading

gabrielliwerant commented Sep 19, 2019

patorjk commented Sep 19, 2019

gabrielliwerant commented Sep 25, 2019

patorjk commented Sep 25, 2019

Feature/issue 871 (setCellHeaderProps) and Feature/issue 714 (setTableProps) #889

Feature/issue 871 (setCellHeaderProps) and Feature/issue 714 (setTableProps) #889

Conversation

patorjk commented Aug 30, 2019

coveralls commented Aug 30, 2019 • edited Loading

patorjk commented Sep 11, 2019

gabrielliwerant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielliwerant commented Sep 19, 2019 • edited Loading

gabrielliwerant commented Sep 19, 2019

patorjk commented Sep 19, 2019

gabrielliwerant commented Sep 25, 2019

patorjk commented Sep 25, 2019

coveralls commented Aug 30, 2019 •

edited

Loading

gabrielliwerant commented Sep 19, 2019 •

edited

Loading