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

Fixes: #2851 : unnecessary pagination #2905

Closed
wants to merge 2 commits into from

Conversation

anishagg17
Copy link
Contributor

@anishagg17 anishagg17 commented Feb 22, 2020

Summary

Improved EuiPagination so that it does not appear unnecessarily
Fixes: #2851

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in IE11 and Firefox
  • Checked for breaking changes and labeled appropriately
  • 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?

@cla-checker-service
Copy link

cla-checker-service bot commented Feb 22, 2020

💚 CLA has been signed

@anishagg17 anishagg17 requested review from cchaos and chandlerprall and removed request for cchaos February 22, 2020 19:40
@chandlerprall
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

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

@chandlerprall
Copy link
Contributor

chandlerprall commented Feb 22, 2020

Given #2851 is a bug in the Canvas app and not EUI, this change is only a stylistic one in nature, difference can be see in the docs' first In-Memory Table section (screen shots of difference below).

Adding @cchaos as reviewer.

Existing

https://elastic.github.io/eui/#/tabular-content/tables
existing pagination, has left/right buttons

Updated

https://eui.elastic.co/pr_2905/#/tabular-content/tables
new pagination, without left/right buttons

This PR also introduces a bug when no data is provided, which throws Cannot read property 'length' of undefined at if (pages.length > 1) {. This is reproduced in the In-Memory Table - Selection docs section.

@cchaos
Copy link
Contributor

cchaos commented Feb 24, 2020

We had actually talked about this functionality recently and extensively in a different PR #2598 and had made the decision that the pagination controls should still show even if there is only 1 page of results. This means keeping in the arrow buttons, though they would be disabled.

Keeping the arrows (though disabled) indicates to the user what the number 1 represents (paging). Otherwise, there is no context as to what that number indicates. Is it number of results or number of pages.

Since it was found that #2851 was actually a bug in a consuming application, it should have actually been closed. I would opt for not changing the current functionality. Though we definitely appreciate your work on this @anishagg17 , I don't think there is actually a problem to solve here and as @chandlerprall mentioned, this also introduces a bug/error to be thrown.

@anishagg17
Copy link
Contributor Author

@cchaos I think we should get rid of this

Screenshot 2020-02-24 at 10 00 51 PM

@cchaos
Copy link
Contributor

cchaos commented Feb 24, 2020

@anishagg17 Can you explain further?

@anishagg17
Copy link
Contributor Author

I mean that these icons shouldn't render if there is only a single page

@cchaos
Copy link
Contributor

cchaos commented Feb 24, 2020

That is our mobile version of the pagination bar (simply removes the numbers). There's a lot of speculation with how we can make this experience better, however, I don't think just removing them is the right answer. Again, this was discussed in the PR #2598, and we've decided that our current UI display is how we want to handle these situations.

We appreciate your time and feedback on this issue, but I will be closing the issue as we feel we already have the experience we want.

@anishagg17 anishagg17 closed this Feb 24, 2020
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.

Infinite pagination in empty EuiBasicTable
4 participants