[DataGrid] Fix display of row count and selected rows on mobile #508

DanailH · 2020-10-29T14:28:15Z

Fixes #440

This PR aims to solve the issue of having the Total Rows and rows selected counts hidden on small screens. I didn't change the styling but only made the 2 files visible.

oliviertassinari

It doesn't fit on mobile with pagination:

https://deploy-preview-508--material-ui-x.netlify.app/components/data-grid/pagination/

DanailH · 2020-10-29T15:01:29Z

It doesn't fit on mobile with pagination:

https://deploy-preview-508--material-ui-x.netlify.app/components/data-grid/pagination/

Yes about that, I saw it (its event worst if you select a row) but I'm not sure how to handle it design-wise. Maybe we can have a discussion about it. The fix will be easy to implement. What do you think @oliviertassinari ?

oliviertassinari · 2020-10-29T15:03:15Z

What about we make it use two rows if the features are enabled? cc @dtassone

DanailH · 2020-10-29T15:07:41Z

That would definitely solve the issue but with the tradeoff that the footer height will increase and if you have set your grid to height to a smaller number there may bot be enough height for the visible part (although I'm not sure how valid of an argument that is since setting the height of the Grid is done by the user)

oliviertassinari · 2020-10-29T15:14:12Z

Oh right, in this case, maybe we can have at most one footer "row":

pagination wins over selected row & row count.
If pagination={false} hideFooterRowCount hideFooterSelectedRowCount => hide the footer.

dtassone · 2020-10-29T15:19:16Z

How about we hide a few things in the pagination component.
So we just display 2 buttons to navigate bw pages

dtassone · 2020-10-29T15:30:31Z

For the info about rows we can change it as well, reduce the wordings or put it on 2 rows

200 Rows, 1 selected

oliviertassinari · 2020-10-29T15:33:09Z

For the info about rows we can change it as well, reduce the wordings or put it on 2 rows

The wordings will need to be translated with an API, it will increase it. It might also still overflow in another locale, so I would vote against for simplicity. Can a mobile user leverage the information?

dtassone · 2020-10-29T15:44:26Z

For the info about rows we can change it as well, reduce the wordings or put it on 2 rows

The wordings will need to be translated with an API, it will increase it. It might also still overflow in another locale, so I would vote against for simplicity. Can a mobile user leverage the information?

Yeah maybe just the total rows on mobile. Also can we put only 2 btns for pagination?

DanailH · 2020-10-29T16:16:55Z

I'll play around and create a couple of options and attach them here as screenshots to see which one will look the best

oliviertassinari · 2020-10-29T17:50:31Z

Mayby hiding the rows per page would be enough. The best case is if we can manage the logic with CSS only, so its easier to customize (as opposed to changing the DOM).

DanailH · 2020-10-30T11:46:14Z

Ok I did some tests and it seems that the easies and most secure option is what @oliviertassinari suggested - hide the selected row and total row count. The reason is that the for the footer we are using the TablePagination from core and changing that will be tricky. The other option is to implement our own DataGrid footer component. I'm open for suggestions :D

oliviertassinari · 2020-10-30T12:02:58Z

@DanailH Sounds good, so and on desktop, we leave it untouched.

dtassone · 2020-10-30T12:04:04Z

Ok I did some tests and it seems that the easies and most secure option is what @oliviertassinari suggested - hide the selected row and total row count. The reason is that the for the footer we are using the TablePagination from core and changing that will be tricky. The other option is to implement our own DataGrid footer component. I'm open for suggestions :D

How about we keep what is circled in orange only?

The row counter is actually displayed already in the pagination

DanailH · 2020-10-30T13:38:48Z

@DanailH Sounds good, so and on desktop, we leave it untouched.

Sure, we can do that. Currently, it hides it also on desktop but I can easily change it 😉

DanailH · 2020-10-30T13:46:35Z

Ok I did some tests and it seems that the easies and most secure option is what @oliviertassinari suggested - hide the selected row and total row count. The reason is that the for the footer we are using the TablePagination from core and changing that will be tricky. The other option is to implement our own DataGrid footer component. I'm open for suggestions :D

How about we keep what is circled in orange only?

The row counter is actually displayed already in the pagination

This change will still require to update/change the TablePagination component from core because the rows per page section is in it and there is no API to hide it or at least I cant' see one -> https://material-ui.com/api/table-pagination/

dtassone · 2020-10-30T13:47:27Z

Ok I did some tests and it seems that the easies and most secure option is what @oliviertassinari suggested - hide the selected row and total row count. The reason is that the for the footer we are using the TablePagination from core and changing that will be tricky. The other option is to implement our own DataGrid footer component. I'm open for suggestions :D

How about we keep what is circled in orange only?

The row counter is actually displayed already in the pagination

This change will still require to update/change the TablePagination component from core because the rows per page section is in it and there is no API to hide it or at least I cant' see one -> https://material-ui.com/api/table-pagination/

You can do it with CSS

dtassone · 2020-10-30T13:47:52Z

You can also display the grid row counter only when there is no pagination

DanailH · 2020-10-30T14:02:39Z

Yes, that's what I'm doing now. If the pagination is available the Total rows and Rows selected are hidden.

DanailH · 2020-10-30T14:04:36Z

Ok I did some tests and it seems that the easies and most secure option is what @oliviertassinari suggested - hide the selected row and total row count. The reason is that the for the footer we are using the TablePagination from core and changing that will be tricky. The other option is to implement our own DataGrid footer component. I'm open for suggestions :D

How about we keep what is circled in orange only?

The row counter is actually displayed already in the pagination

This change will still require to update/change the TablePagination component from core because the rows per page section is in it and there is no API to hide it or at least I cant' see one -> https://material-ui.com/api/table-pagination/

You can do it with CSS

True, but should we do it like this, hack our own components. It's better to introduce a prop in the TablePagination to hide then. I can do it, it's not a problem @oliviertassinari ?

oliviertassinari · 2020-10-30T14:54:42Z

@DanailH The data grid relies on core v4 so far, we can't expect any changes to land in it. However, the table pagination exposes enough public API, so no need to hack around.

…m TablePagination

DanailH · 2020-11-02T16:17:59Z

packages/grid/_modules_/grid/components/pagination.tsx

@@ -11,6 +12,16 @@ export interface PaginationComponentProps {
  rowsPerPageOptions?: number[];
 }

+// Used to hide the drop down select from the TablePaginagion
+const useStyles = makeStyles({


Ok, I've updated the logic with what we discussed during the planning today, here is the summary:

Remove Total Rows when the pagination is available

Remove the select and the label inside the TablePagination

Update the login so the selected rows is only shown on desktop when the pagination is available

Adjust the footer items alignment when there is only pagination (it should be aways on the right side)

Last but not least a question - these styles are needed to hide the select from the TablePagination component but I'm not sure it this is the best place to keep them, ideas ?

DanailH · 2020-11-02T16:22:10Z

Hmm I guess the flaky test is this one Chrome 84.0.4147.89 (Mac OS 10.15.6) <XGrid /> keyboard cell navigation with arrows FAILED 😅

oliviertassinari · 2020-11-02T17:12:26Z

@DanailH CircleCI allows to rerun test from fails. I have launch it.

oliviertassinari

I wonder how to best test this. Maybe DOM and class name assertions would be ideal.

packages/grid/_modules_/grid/components/pagination.tsx

oliviertassinari · 2020-11-02T17:14:51Z

packages/grid/_modules_/grid/components/default-footer.tsx

+        ref={ref}
+        className={classnames({
+          'MuiDataGrid-footer-pagination-visible': isPaginationAvailable,
+          'MuiDataGrid-footer-justifyContent-right': justifyItemsRight,


Adding a hypen for separating internal components was one thing, doing it for naming is another.

Suggested change

'MuiDataGrid-footer-justifyContent-right': justifyItemsRight,

'MuiDataGrid-footer-justifyContentRight': justifyItemsRight,

Instead of "right" using "end" would be better for RTL.

What if we render an empty div instead so we don't have to apply a custom class name?

Or we could also inverse the direction of the flex container. That could be even better if that's what most people would want, not sure.

I was thinking of rendering an empty div as well but I didn't like how it looked. Maybe we can reintroduce it. The benefit is that if we add a single empty div around the row selected we don't need to add any additional CSS logic/styles.

oliviertassinari · 2020-11-02T17:18:37Z

packages/grid/_modules_/grid/components/styled-wrappers/GridRootStyles.ts

+            '& .MuiDataGrid-rowCount, & .MuiDataGrid-selectedRowCount': {
+              visibility: 'hidden',
+              [theme.breakpoints.up('md')]: {
+                visibility: 'visible',


Why not using display none?

I need it to take space. The reason is that we want the pagination to always be right-aligned. If I make it display: none then the logic to aways align the pagination on the right will be unnecessarily complex.

packages/grid/_modules_/grid/components/default-footer.tsx

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

…ailH/material-ui-x into fix/DataGrid-440-footer-issue-mobile

oliviertassinari · 2020-11-03T12:24:45Z

I wonder if we shouldn't use the breakpoint before md (960px) but sm (600px). People can customize it but that still seems to be common enough per mui/material-ui#21902. It's also what we use in the main repo (sm).

DanailH · 2020-11-03T12:30:55Z

I wonder if we shouldn't use the breakpoint before md (960px) but sm (600px). People can customize it but that still seems to be common enough per mui-org/material-ui#21902. It's also what we use in the main repo (sm).

I just went ahead and changed it to sm. This is the only place where we use a breakpoint and with the new changes (we hide part of the TablePagination) there is enough space to have both parts on a single line before hitting sm resolution

oliviertassinari

We can no longer change the number of rows per page on desktop, maybe use CSS to target the first .caption style on mobile?
Something is strange with this demo:

https://deploy-preview-508--material-ui-x.netlify.app/components/data-grid/pagination/#auto-size

DanailH · 2020-11-03T18:57:52Z

We can no longer change the number of rows per page on desktop, maybe use CSS to target the first .caption style on mobile?

Something is strange with this demo:

https://deploy-preview-508--material-ui-x.netlify.app/components/data-grid/pagination/#auto-size

I'll check the demo. Regarding the other point - as far as I understood that was the desired behaviour from our discussion, to hide both the label and the select? I can only hide the label and keep the select.

oliviertassinari · 2020-11-03T19:27:46Z

I'll check the demo. Regarding the other point - as far as I understood that was the desired behaviour from our discussion, to hide both the label and the select? I can only hide the label and keep the select.

I was under the assumption that it was only on mobile to save space. From my perspective, if we were to change this on desktop too, it would be the type of change that should be done at the TablePagination level, not in the data grid.

DanailH · 2020-11-04T08:55:37Z

I'll check the demo. Regarding the other point - as far as I understood that was the desired behaviour from our discussion, to hide both the label and the select? I can only hide the label and keep the select.

I was under the assumption that it was only on mobile to save space. From my perspective, if we were to change this on desktop too, it would be the type of change that should be done at the TablePagination level, not in the data grid.

Ok, hopefully, the last update - I've only hidden the Rows per page + the select from mobile resolutions, they are now visible on desktop.

The drawback - I had to return the media query to md because if you have a row selected and the pagination on small but not telephone screen resolution the rows selected falls on 2 lines and a scroll may appear.

) * Enable Row Count and Selected Rows on mobile * Hide RowCount and SelectedRowCount if paginationComponent is provided * Hide Row count and Selected rows on mobile if Pagination is available * Hide Total rows when there is pagination, remove select and label from TablePagination * Update packages/grid/_modules_/grid/components/pagination.tsx Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com> * Adjust class names * Swap Total Row and rows selected places * Update footer breakpoint to "sm" * Fix formatting * Fix responsive styles Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

Enable Row Count and Selected Rows on mobile

335070f

DanailH requested review from dtassone and oliviertassinari October 29, 2020 14:28

DanailH self-assigned this Oct 29, 2020

oliviertassinari changed the title ~~Enable Row Count and Selected Rows on mobile~~ [DataGrid] Fix display of row count and selected rows on mobile Oct 29, 2020

oliviertassinari added bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! labels Oct 29, 2020

oliviertassinari reviewed Oct 29, 2020

View reviewed changes

Hide RowCount and SelectedRowCount if paginationComponent is provided

919c120

Hide Row count and Selected rows on mobile if Pagination is available

8df8e2d

Hide Total rows when there is pagination, remove select and label fro…

1c2833d

…m TablePagination

DanailH commented Nov 2, 2020

View reviewed changes

oliviertassinari reviewed Nov 2, 2020

View reviewed changes

DanailH and others added 3 commits November 3, 2020 09:54

Update packages/grid/_modules_/grid/components/pagination.tsx

96d1b28

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

Adjust class names

538944f

Merge branch 'fix/DataGrid-440-footer-issue-mobile' of github.com:Dan…

0bcad82

…ailH/material-ui-x into fix/DataGrid-440-footer-issue-mobile

DanailH requested a review from oliviertassinari November 3, 2020 09:11

DanailH added 2 commits November 3, 2020 13:26

Swap Total Row and rows selected places

d2df57b

Update footer breakpoint to "sm"

5bc8d58

Fix formatting

a00770a

oliviertassinari requested changes Nov 3, 2020

View reviewed changes

Fix responsive styles

89d373b

DanailH requested a review from oliviertassinari November 4, 2020 09:05

oliviertassinari approved these changes Nov 4, 2020

View reviewed changes

dtassone approved these changes Nov 4, 2020

View reviewed changes

DanailH merged commit 1071158 into mui:master Nov 4, 2020

oliviertassinari mentioned this pull request Jan 13, 2021

[DataGrid] Footer count not showing in "official" demo itself #847

Closed

2 tasks

	'MuiDataGrid-footer-justifyContent-right': justifyItemsRight,
	'MuiDataGrid-footer-justifyContentRight': justifyItemsRight,

[DataGrid] Fix display of row count and selected rows on mobile #508

[DataGrid] Fix display of row count and selected rows on mobile #508

Conversation

DanailH commented Oct 29, 2020

oliviertassinari left a comment

Choose a reason for hiding this comment

DanailH commented Oct 29, 2020

oliviertassinari commented Oct 29, 2020 • edited

DanailH commented Oct 29, 2020

oliviertassinari commented Oct 29, 2020 • edited

dtassone commented Oct 29, 2020

dtassone commented Oct 29, 2020

oliviertassinari commented Oct 29, 2020 • edited

dtassone commented Oct 29, 2020

DanailH commented Oct 29, 2020

oliviertassinari commented Oct 29, 2020

DanailH commented Oct 30, 2020

oliviertassinari commented Oct 30, 2020

dtassone commented Oct 30, 2020

DanailH commented Oct 30, 2020 • edited

DanailH commented Oct 30, 2020

dtassone commented Oct 30, 2020

dtassone commented Oct 30, 2020

DanailH commented Oct 30, 2020

DanailH commented Oct 30, 2020

oliviertassinari commented Oct 30, 2020

DanailH Nov 2, 2020

Choose a reason for hiding this comment

DanailH commented Nov 2, 2020

oliviertassinari commented Nov 2, 2020

oliviertassinari left a comment

Choose a reason for hiding this comment

oliviertassinari Nov 2, 2020

Choose a reason for hiding this comment

oliviertassinari Nov 2, 2020

Choose a reason for hiding this comment

oliviertassinari Nov 2, 2020

Choose a reason for hiding this comment

oliviertassinari Nov 2, 2020

Choose a reason for hiding this comment

DanailH Nov 3, 2020 • edited

Choose a reason for hiding this comment

oliviertassinari Nov 2, 2020

Choose a reason for hiding this comment

DanailH Nov 3, 2020

Choose a reason for hiding this comment

oliviertassinari commented Nov 3, 2020 • edited

DanailH commented Nov 3, 2020

oliviertassinari left a comment • edited

Choose a reason for hiding this comment

DanailH commented Nov 3, 2020

oliviertassinari commented Nov 3, 2020

DanailH commented Nov 4, 2020

oliviertassinari commented Oct 29, 2020 •

edited

oliviertassinari commented Oct 29, 2020 •

edited

oliviertassinari commented Oct 29, 2020 •

edited

DanailH commented Oct 30, 2020 •

edited

DanailH Nov 3, 2020 •

edited

oliviertassinari commented Nov 3, 2020 •

edited

oliviertassinari left a comment •

edited