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.

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

[EuiDataGrid][EuiTables] Fix pagination potentially rendering out view on narrow tables with many pages #5561

Merged
merged 8 commits into from
Jan 31, 2022

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Jan 25, 2022

Summary

This attempts to address item 5 in #4458:

  1. It looks like we also need a smarter pagination display on skinny screens/containers.

Repro steps:

  1. Go to https://elastic.github.io/eui/#/tabular-content/data-grid-virtualization
  2. Click on page 200 in the first example
  3. Click on page 196
  4. Note how the rightmost arrow is now cut off due to the small width of the grid and the high amount of pages/content

Before

before

After

The overflowing pagination UI can now be scrolled to.

after

Checklist

- [ ] Check against all themes for compatibility in both light and dark modes

  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox

- [ ] Props have proper autodocs and playground toggles
- [ ] Added documentation
- [ ] Checked Code Sandbox works for any docs examples

- [ ] Checked for breaking changes and labeled appropriately

  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

- adding xScroll at least allows users to access the overflowing content
@chandlerprall chandlerprall requested review from miukimiu and removed request for chandlerprall January 25, 2022 22:44
@@ -121,6 +121,7 @@ export class EuiTablePagination extends Component<
justifyContent="spaceBetween"
alignItems="center"
responsive={false}
className="eui-xScroll"
Copy link
Member Author

@cee-chen cee-chen Jan 25, 2022

Choose a reason for hiding this comment

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

Just wanted to note while this isn't the most "smart" solution, I think it's the one with the best tradeoffs.

The "smartest" pagination would be one that has a resize observer on itself and the underlying EuiPagination to make sure the width of one doesn't exceed the right bound of the other, but I strongly don't think the extra listeners and perf overhead are worth it vs. a simple overflow-x: auto;.

IMO, extremely small tables/grids are an edge case that we don't need to add a bunch of code for, and should mostly be caught by the responsive behavior of EuiPagination. Providing some way of accessing the overflowing UI via scrollbars should be sufficient until the day container queries become fully standard and we can just use those instead 🙏

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5561/

1 similar comment
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5561/

Copy link
Contributor

@miukimiu miukimiu left a comment

Choose a reason for hiding this comment

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

Thanks @constancecchen! 🎉

It's looking great. I just found some issues in Safari 15.2:

Issue 1 in Safari

When clicking a pagination number the .euiDataGridBody outline appears and we see a black border on top of the pagination. I think is safe to set the outline to none.

Screenshot 2022-01-28 at 12 55 00

Issue 2 in Safari

Following you suggestion:

Click on page 200 in the first example
Click on page 196

The arrow right gets cutt off when we reach the end of the horizontal overflow as we can see on the following screenshot:

Screenshot 2022-01-28 at 12 54 28

@cee-chen
Copy link
Member Author

cee-chen commented Jan 28, 2022

The black outline on Safari actually occurs on all data grids (including the main example) in production and isn't related to this specific PR - the underlying cause is likely in our focus JS. Is it okay if I file a separate issue for it and tackle it in another follow-up PR? EDIT: #5577

2nd issue is definitely valid though. Safari and flexbox - name a more iconic garbage duo 😑 I'll look into a fix!

@cee-chen
Copy link
Member Author

cee-chen commented Jan 28, 2022

@miukimiu I looked more into the Safari issue and it appears to be caused by the negative margins we use in our EuiFlexGroup gutter CSS (#5575). Because this issue would apply to all instances of EuiFlexGroup that are being scrolled horizontally in Safari, I think it's better solved at the higher EuiFlexGroup level rather than in this PR.

Would you feel okay also opening a separate issue for the problem and fixing it outside this PR? If not, the approach I would likely take is to rewrite the table pagination component completely to use custom flex CSS instead of the EuiFlexGroup components. Totally up to you though.

EDIT: In the meanwhile I've also reduced the gutter size on the flex group, which helps make the right arrow less cut off on Safari.

- for better small width/scroll behavior and to reduce Safari cutoff
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5561/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5561/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5561/

@miukimiu miukimiu self-requested a review January 31, 2022 17:24
Copy link
Contributor

@miukimiu miukimiu left a comment

Choose a reason for hiding this comment

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

Thanks, @constancecchen.

Now we can see all the pagination items and that's the purpose of this PR. In Safari the black outline it's fixed (#5578) and the arrow is not being cut anymore.

So LGTM! 🎉

I'll open a new issue to see if we can have a better pagination design. The EuiFlexGroup gutter CSS (#5575) might improve a little bit the design but not for all scenarios.

@cee-chen
Copy link
Member Author

Thanks a million @miukimiu!!

@cee-chen cee-chen merged commit 2e285e8 into elastic:main Jan 31, 2022
@cee-chen cee-chen deleted the datagrid/4458/pagination branch January 31, 2022 17:54
gdimitropoulos pushed a commit to gdimitropoulos/eui that referenced this pull request Apr 21, 2022
…w on narrow tables with many pages (elastic#5561)

* Fix pagination rendering items out of view on small tables

- adding xScroll at least allows users to access the overflowing content

* Add changelog entry

* Reduce gutter size
- for better small width/scroll behavior and to reduce Safari cutoff

* Update snapshots
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants