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: fixed Rows per page controls disappear when there's only a single page #2978

Merged
merged 7 commits into from
Mar 6, 2020

Conversation

anishagg17
Copy link
Contributor

@anishagg17 anishagg17 commented Mar 4, 2020

Summary

Fixes : #2972
fixed Rows per page controls disappear when there's only a single page

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in IE11 and Firefox
  • Props have proper autodocs
  • Improved documentation examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@anishagg17
Copy link
Contributor Author

@cchaos it was just a minor logical error in the onChangeItemsPerPage function of doc's

@cchaos
Copy link
Contributor

cchaos commented Mar 4, 2020

Jenkins, test this

@kibanamachine
Copy link

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

@cchaos cchaos changed the title fixed Rows per page controls disappear when there's only a single page [Docs] EuiDataGrid: fixed Rows per page controls disappear when there's only a single page Mar 4, 2020
@cchaos
Copy link
Contributor

cchaos commented Mar 4, 2020

Thanks @anishagg17, this does fix part of the issue described in #2972 (but only for that one example). Could you update the rest of the examples to behave the same? Unless, @chandlerprall, you think there's a way for EuiDataGrid to handle this directly?

The other half of the issue is that EuiDataGrid, hides all pagination controls when there's only a single page. Meaning, once I've decided to display all 100 results per page, I can't get back to displaying only 25 per page.

Screen Recording 2020-03-04 at 11 02 AM

@anishagg17
Copy link
Contributor Author

anishagg17 commented Mar 4, 2020

The other half of the issue is that EuiDataGrid, hides all pagination controls when there's only a single page. Meaning, once I've decided to display all 100 results per page, I can't get back to displaying only 25 per page.

I too noticed that issue and will soon fix that issue too.

@chandlerprall
Copy link
Contributor

@chandlerprall, you think there's a way for EuiDataGrid to handle this directly?

Yep, the problematic logic is at

if (pageCount === 1) {

@anishagg17
Copy link
Contributor Author

@cchaos now both issues are fixed , you may have a look .

@chandlerprall you think there's a way for EuiDataGrid to handle this directly?

Don't you think users should have complete flexibility of their own and should reset pageIndex according to their need

@cchaos
Copy link
Contributor

cchaos commented Mar 5, 2020

Jenkins, test this

@kibanamachine
Copy link

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

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Don't you think users should have complete flexibility of their own and should reset pageIndex according to their need

EuiDataGrid was built with that being the intention - the application can decide whether or not to reset the current page. Best practice is to reset (so thank you for updating the examples to act that way), but we've had the opposite functionality requested (or at least brought up) around EUIs tables, so the option is left to the application. So, long answer short, yes.


if (pageCount === 1) {
return null;
newPageSizeOptions = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we want to modify the page size options, for a few reasons:

  • the data may be filtered outside the grid, e.g. there's a search box on the page that is used to load new data; data sets of different sizes would change the availability of page sizes
  • this logic almost enforces pagination, as it keeps the page size under the total number of pages
  • an extension of the previous point, this artificial limiting would only apply once the pageCount is 1, meaning the available sizes could change after the user interacts with the original set of page sizes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chandlerprall should I just return the normal pagination if the page count is 1 also

@anishagg17
Copy link
Contributor Author

@chandlerprall I noticed in my application that in
Data grid schemas and popovers section the table is passed pageSizeOptions : [5,10,25]
but it only has 5 items , and when page-size is switched to 25 their is no change in the table so I think it would be fine if we let pageSizeOptions unaltered and let is pass through the pagination even in case of a single page also.

@anishagg17 anishagg17 requested a review from chandlerprall March 6, 2020 11:48
const pageCount = Math.ceil(props.rowCount / pageSize);

if (pageCount === 1) {
if (props.rowCount === 0) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have removed pagination only if there is no content to render

@chandlerprall
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

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

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Changes LGTM; Pulled & tested locally.

@cchaos are you good with the idea of always showing pagination except when the grid is told there are 0 rows of content?

@cchaos cchaos changed the title [Docs] EuiDataGrid: fixed Rows per page controls disappear when there's only a single page EuiDataGrid: fixed Rows per page controls disappear when there's only a single page Mar 6, 2020
@cchaos
Copy link
Contributor

cchaos commented Mar 6, 2020

Hey @anishagg17 , @chandlerprall & @snide & I talked around this a bit and decided that the best experience for data grid users would be to only show the pagination controls IF the length of the data (total number of rows) is less than the lowest number of rows per page.

For example, since 10 is the lowest number in this list:

Image 2020-03-06 at 2 04 13 PM

Then the pagination controls will hide if the table contains less than 10 rows.

Can you update the logic to support this?

@anishagg17
Copy link
Contributor Author

@cchaos Done !

@cchaos
Copy link
Contributor

cchaos commented Mar 6, 2020

Jenkins, test this

@kibanamachine
Copy link

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

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Great, I think this works well! Just cleaned up the changelog language a bit

CHANGELOG.md Outdated Show resolved Hide resolved
@snide snide merged commit 4427816 into elastic:master Mar 6, 2020
@snide
Copy link
Contributor

snide commented Mar 6, 2020

Thanks for the PR!

@anishagg17
Copy link
Contributor Author

Thanks for merging @snide

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.

[Data Grid] Rows per page controls disappear when there's only a single page
5 participants