[Table] Add a TableFooter for pagination #8254

leMaik · 2017-09-18T00:36:14Z

This PR adds a new TableFooter component and extends the EnhancedTable demo to demonstrate pagination. It also adds the missing Android versions to the nutrition list. 🍰 🍭

Specs:

This PR:

Add a TableFooter, similar to TableHead
Add pixel-perfect pagination footer for tables
Add tests for TableFooter
Move the pagination code into a TablePagination component as suggested in [Table] Add support for pagination #7746 (comment) and immediately add tests this time
Refactor TablePagination to a class and fix a bug when increasing the rows per page results in displaying pages that don't exist anymore
Add TypeScript typings

Closes #7746

This also adds more demo data, and yes, the nutrition information of Marshmallow, Nougat and Oreo is actually real.

oliviertassinari

I'm happy to see this PR :).
I have only noticed one important issue. The checkbox logic seems to be broken with the pagination. For instance, try selection everything. The top checkbox won't be checked.

oliviertassinari · 2017-09-18T20:18:50Z

docs/src/pages/demos/tables/EnhancedTable.js

@@ -148,6 +154,118 @@ EnhancedTableToolbar.propTypes = {

 EnhancedTableToolbar = withStyles(toolbarStyles)(EnhancedTableToolbar);

+const footerStyles = theme => ({
+  cell: {
+    padding: '0 !important',


What's the important for? It's the first introduction of such pattern. It shouldn't be needed. And if we do, something is wrong.

Maybe something is wrong, because something forced the cell to keep having padding even if I set padding to zero here. !important fixed this. I'll look into that later.

I can have a look at this.

I'd really appreciate your help on this, I'll focus on the other things in the meantime. 👍

oliviertassinari · 2017-09-18T20:19:09Z

docs/src/pages/demos/tables/EnhancedTable.js

+    flex: '1 1 100%',
+  },
+  select: {
+    marginLeft: 8,


don't hesitate to use theme.spacing.unit.

oliviertassinari · 2017-09-18T20:20:01Z

docs/src/pages/demos/tables/EnhancedTable.js

+  },
+});
+
+let EnhancedTableFooter = props => {


It's the first introduction of an anonymous arrow function, please stick to the function convention we use.

It isn't, look at EnhancedTableToolbar. 😉

🙈 oh nooo entropyyyy -sum p*log(p)

Don't worry, I'll change both.

I know why this is an anonymous arrow function! To apply the styles, the variable is re-assigned, which throws a linter error (no-func-assign) if they are functions.
What to do now?

oliviertassinari · 2017-09-18T20:20:48Z

docs/src/pages/demos/tables/EnhancedTable.js

+              value={rowsPerPage}
+              onChange={onChangeRowsPerPage}
+            >
+              {rowsPerPageOptions.map(v => (


It would be better to use an explicit variable name instead of v.

oliviertassinari · 2017-09-18T20:22:04Z

src/Table/TableFooter.js

+
+type AllProps = DefaultProps & Props;
+
+class TableFooter extends React.Component<AllProps, void> {


Is void needed? I thought that React.Component<AllProps> was enough. I could be wrong, I'm not much involved into flow.

I'm pretty sure that I didn't write that but just copied this ~~or the prettier added this~~. 🤔

Edit: It's React.Component<AllProps, void> in the other Table components, too.

oliviertassinari · 2017-09-18T20:43:28Z

Move the pagination code into a TablePagination

@leMaik What do you have in mind for this abstraction? For instance. How do you want to design the API?

leMaik · 2017-09-18T21:31:58Z

src/Table/TableFooter.js

+    component: 'tfoot',
+  };
+
+  getChildContext() {


Same as in TableHead, it makes a TableRow know that it is nested inside a TableFooter so that it doesn't show the bottom line.

leMaik · 2017-09-18T21:35:46Z

@oliviertassinari Basically, I'd just move the EnhancedTableFooter and rename and refactor it a bit. I'll also add a way to customize the strings (for i18n). I really care about the design and layout to look as shown in the specs, so I'd rather not make this too flexible for now.

oliviertassinari · 2017-09-18T21:37:46Z

@leMaik Sounds good. I have found that having placeholders is what provides a good correctness/flexibility ratio.

…rding to the review

leMaik · 2017-09-18T22:02:09Z

@oliviertassinari Fixed the things you mentioned in the review, now working on tests and refactoring. ⚙️

Regarding flexibility: After all, the entire footer can be swapped out, this component just adds one way of doing pagination. It can be seen as a pretty big placeholder already.

leMaik · 2017-09-18T22:20:57Z

Just found another major issue thanks to argos: I completely broke the sorting. 😅
Edit: Fixed

…fix the sorting order in the EnhancedTable demo

leMaik · 2017-09-19T10:54:08Z

@oliviertassinari I'm done with this so far, would you mind taking another look at the PR? CI doesn't want to run because I increased the build size, but obviously adding a component increases it. 😅

codecov-io · 2017-09-19T20:28:29Z

Codecov Report

Merging #8254 into v1-beta will decrease coverage by 0.08%.
The diff coverage is 87.5%.

@@             Coverage Diff             @@
##           v1-beta    #8254      +/-   ##
===========================================
- Coverage    99.42%   99.33%   -0.09%     
===========================================
  Files          147      149       +2     
  Lines         2242     2258      +16     
===========================================
+ Hits          2229     2243      +14     
- Misses          13       15       +2

Impacted Files	Coverage Δ
src/Table/TableCell.js	`100% <ø> (ø)`	⬆️
src/Table/TableFooter.js	`100% <100%> (ø)`
src/Table/TablePagination.js	`77.77% <77.77%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b4b53a...32008a9. Read the comment docs.

oliviertassinari · 2017-09-19T20:32:30Z

docs/src/pages/demos/tables/EnhancedTable.js

@@ -178,9 +191,10 @@ class EnhancedTable extends React.Component {
      order = 'asc';
    }

-    const data = this.state.data.sort(
-      (a, b) => (order === 'desc' ? b[orderBy] > a[orderBy] : a[orderBy] > b[orderBy]),


Yeah, I think that the logic was broken 👍

It returned true and false instead of a negative or positive number.

oliviertassinari · 2017-09-19T20:33:19Z

docs/src/pages/demos/tables/EnhancedTable.js

@@ -220,11 +234,19 @@ class EnhancedTable extends React.Component {
    this.setState({ selected: newSelected });
  };

+  handleChangePage = (e, page) => {


💣 Please stick to verbose variable names: event.

oliviertassinari · 2017-09-19T21:11:51Z

@leMaik Excellent work so far. I think that we only miss the typescript definition of TablePagination and some test coverage to merge this PR 💃 .

leMaik · 2017-09-19T21:15:53Z

@oliviertassinari Currently working on the test coverage and the bug that argos just discovered (EnhancedTable is not sorted after mounting).
Look at the last item in my TODO list above, there's still some work left but we're getting there. :)

oliviertassinari · 2017-09-19T21:22:34Z

@leMaik Oh ok, feel free to continue the PR then :). If you plan to keep opening great PRs like this one here, I will give you the push right access to the repository. It can make the process easier.

…hanging the rows per page, lint the code

leMaik · 2017-09-19T23:07:39Z

@oliviertassinari Could you manually confirm the changes in argos? The table is now properly sorted, it just looks different because there are new sweets.

I added more tests and typings, so now I'm really actually done as long as you don't have any further suggestions. 🎉

If you plan to keep opening great PRs like this one here, I will give you the push right access to the repository. It can make the process easier.

I feel honored. :) While I do plan to keep contributing to material-ui, I can't guarantee the interval of the contributions. 😅

leMaik · 2017-09-19T23:09:40Z

src/Table/TablePagination.js

@@ -16,7 +16,10 @@ import KeyboardArrowRight from '../svg-icons/KeyboardArrowRight';

 export const styles = (theme: Object) => ({
  cell: {
-    padding: '0 !important',
+    // Increase the specificity to override TableCell.
+    '&:last-child': {


Nice! Didn't think about this.

oliviertassinari · 2017-09-20T07:19:56Z

@leMaik Awesome

I feel honored. :)

You do deserve it. You have been, with your team, did a great job: https://mui.wertarbyte.com/. Yes, no pressure :).

leMaik added 2 commits September 18, 2017 02:29

[TableFooter] Add a TableFooter component

efdf86a

[TableFooter] Add a footer for pagination to the enhanced table demo

432e0b9

This also adds more demo data, and yes, the nutrition information of Marshmallow, Nougat and Oreo is actually real.

leMaik mentioned this pull request Sep 18, 2017

[Table] Add support for pagination #7746

Closed

Lint the code

d531823

oliviertassinari reviewed Sep 18, 2017

View reviewed changes

oliviertassinari added component: table This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation v1 labels Sep 18, 2017

oliviertassinari added the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Sep 18, 2017

leMaik commented Sep 18, 2017

View reviewed changes

[EnhancedTable] Fix checkbox in table header, small code changes acco…

e3bd1ac

…rding to the review

[TableFooter] Add tests

4ecb694

leMaik added 2 commits September 19, 2017 00:29

[EnhancedTable] Fix sorting

b6b22a4

[TablePagination] Add a TablePagination component, lint the code and …

d04893c

…fix the sorting order in the EnhancedTable demo

increase size-limit

32008a9

oliviertassinari reviewed Sep 19, 2017

View reviewed changes

some changes

6494f38

oliviertassinari reviewed Sep 19, 2017

View reviewed changes

remove div

99af851

[EnhancedTable] Fix table not being sorted initially

e8dcadc

leMaik added 2 commits September 20, 2017 00:43

[TablePagination] Add more tests, handle too high page number after c…

0ccb9b9

…hanging the rows per page, lint the code

[TablePagination] Add typings

59ff949

leMaik commented Sep 19, 2017

View reviewed changes

Update the list of supported components

82193b7

oliviertassinari merged commit 35989a9 into mui:v1-beta Sep 20, 2017

oliviertassinari added PR: accepted and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Sep 20, 2017

leMaik deleted the table-footer branch September 20, 2017 09:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Table] Add a TableFooter for pagination #8254

[Table] Add a TableFooter for pagination #8254

leMaik commented Sep 18, 2017 •

edited

Loading

oliviertassinari left a comment

oliviertassinari Sep 18, 2017

leMaik Sep 18, 2017

oliviertassinari Sep 18, 2017

leMaik Sep 18, 2017

oliviertassinari Sep 18, 2017

oliviertassinari Sep 18, 2017

leMaik Sep 18, 2017

oliviertassinari Sep 18, 2017 •

edited

Loading

leMaik Sep 18, 2017

leMaik Sep 18, 2017

oliviertassinari Sep 18, 2017

oliviertassinari Sep 18, 2017

leMaik Sep 18, 2017 •

edited

Loading

oliviertassinari commented Sep 18, 2017 •

edited

Loading

leMaik Sep 18, 2017

leMaik commented Sep 18, 2017 •

edited

Loading

oliviertassinari commented Sep 18, 2017 •

edited

Loading

leMaik commented Sep 18, 2017

leMaik commented Sep 18, 2017 •

edited

Loading

leMaik commented Sep 19, 2017

codecov-io commented Sep 19, 2017 •

edited

Loading

oliviertassinari Sep 19, 2017

leMaik Sep 19, 2017

oliviertassinari Sep 19, 2017

oliviertassinari commented Sep 19, 2017 •

edited

Loading

leMaik commented Sep 19, 2017

oliviertassinari commented Sep 19, 2017

leMaik commented Sep 19, 2017 •

edited

Loading

leMaik Sep 19, 2017

oliviertassinari commented Sep 20, 2017


		type AllProps = DefaultProps & Props;

		class TableFooter extends React.Component<AllProps, void> {

[Table] Add a TableFooter for pagination #8254

[Table] Add a TableFooter for pagination #8254

Conversation

leMaik commented Sep 18, 2017 • edited Loading

oliviertassinari 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

oliviertassinari Sep 18, 2017 • edited Loading

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

leMaik Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

oliviertassinari commented Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

leMaik commented Sep 18, 2017 • edited Loading

oliviertassinari commented Sep 18, 2017 • edited Loading

leMaik commented Sep 18, 2017

leMaik commented Sep 18, 2017 • edited Loading

leMaik commented Sep 19, 2017

codecov-io commented Sep 19, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari commented Sep 19, 2017 • edited Loading

leMaik commented Sep 19, 2017

oliviertassinari commented Sep 19, 2017

leMaik commented Sep 19, 2017 • edited Loading

Choose a reason for hiding this comment

oliviertassinari commented Sep 20, 2017

leMaik commented Sep 18, 2017 •

edited

Loading

oliviertassinari Sep 18, 2017 •

edited

Loading

leMaik Sep 18, 2017 •

edited

Loading

oliviertassinari commented Sep 18, 2017 •

edited

Loading

leMaik commented Sep 18, 2017 •

edited

Loading

oliviertassinari commented Sep 18, 2017 •

edited

Loading

leMaik commented Sep 18, 2017 •

edited

Loading

codecov-io commented Sep 19, 2017 •

edited

Loading

oliviertassinari commented Sep 19, 2017 •

edited

Loading

leMaik commented Sep 19, 2017 •

edited

Loading