Hide paginationBar in EuiBasicTable when there is no data #2598

andreadelrio · 2019-12-06T03:54:28Z

Summary

This PR introduces two changes in EuiBasicTable:

Hides pagination when there are no rows.
Hides pagination if the number of rows is less than the minimum passed in pageSizeOptions. For example:

pageSizeOptions = [3, 5, 8]
totalItemCount > 3 -> Display pagination? Yes
totalItemCount <= 3 -> Display pagination? No

Fixes #899

Checklist

- [ ] Checked in dark mode
- [ ] Checked in mobile
- [ ] Checked in IE11 and Firefox
- [ ] Props have proper autodocs
~~- [ ] Added 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

chandlerprall · 2019-12-06T15:43:12Z

Let's hold off on merging this until #2428 (tables conversion to typescript) is in. This should be able to drop into the new pagination_bar.tsx file without issues, but would like to avoid any potential merge conflicts especially as the TypeScript PR is a community contribution.

edit: Note: git understood the conversion of pagination_bar.js -> pagination_bar.tsx as a rename of the file, so I would expect a rebase or merge of master for this PR to automatically adjust to the changes without a conflict.

chandlerprall · 2019-12-06T15:55:56Z

I wonder about the efficacy of this decision. I can envision scenarios where a hidden pagination bar would confuse a user who believes there is more data and that a bug is preventing access, rather than communicating that there is only one table worth of data.

If we do decide this is the right thing to do, I would question if this logic should be in the pagination bar or in any consuming locations (i.e. check the row count against the minimum page size in the table code instead of the pagination bar), as that allows the pagination bar to be used/displayed in any situation where we (or a consuming dev) wants to show a pagination bar with only one page.

@cchaos thoughts? Mostly I want to make sure these questions were/are thought about - I'm fine being wrong as long as the discussion happen(s/ed)!

andreadelrio · 2019-12-06T16:35:22Z

I wonder about the efficacy of this decision. I can envision scenarios where a hidden pagination bar would confuse a user who believes there is more data and that a bug is preventing access, rather than communicating that there is only one table worth of data.

@chandlerprall You bring up a valid point. I spent yesterday going back and forth between hiding the pagination or not in this scenario and see arguments for either direction. However, I just checked a reference that I like (Mint) and they keep the "rows per page" UI element even when there's no enough rows of data. See:

In what cases does the number of rows change? I can think of a) search b) filters, and in this new implementation we would be hiding the pagination which as you said could be confusing.
The one solution I can think of for that would be for the consumer to display something like "Displaying 5 results" (like Mint does) but I understand we want our component to be as intuitive as possible out of the box.

cchaos · 2019-12-06T17:00:08Z

@andreadelrio It would also be helpful to evaluate this decision with screenshots. Otherwise I have to manipulate the example codes in order to view the scenarios.

I am also confused when we talk about pagination vs rows per page. I feel these are either two separate UI's or Pagination encompasses the actual EuiPagination component and the rows per page. Can you clarify those distinctions?

Thinking about it generally, I would assume:

Empty table: (Meaning, it's empty to start, the user has no data at all.) The table itself (should) show a message that it is empty and therefore I agree that the entire Pagination should not be present.
Any data: (Meaning, even if it's just 1 row or searching came up with no results.) Only the EuiPagination should hide but the rows per page should still be visible.

In any case, we will also eventually need some sort of caption explaining how many total results #1237.

andreadelrio · 2019-12-06T17:43:00Z

@andreadelrio It would also be helpful to evaluate this decision with screenshots. Otherwise I have to manipulate the example codes in order to view the scenarios.

I am also confused when we talk about pagination vs rows per page. I feel these are either two separate UI's or Pagination encompasses the actual EuiPagination component and the rows per page. Can you clarify those distinctions?

Thinking about it generally, I would assume:

Empty table: (Meaning, it's empty to start, the user has no data at all.) The table itself (should) show a message that it is empty and therefore I agree that the entire Pagination should not be present.

Any data: (Meaning, even if it's just 1 row or searching came up with no results.) Only the EuiPagination should hide but the rows per page should still be visible.

In any case, we will also eventually need some sort of caption explaining how many total results #1237.

@cchaos So far I’ve been using "Pagination" to refer to PaginationBar which includes two elements: itemsPerPagePopover and EuiPagination. I think this that you pointed out (that we are dealing with two different elements) makes it more clear for me which way we should go. I say we stay with the current way we handle PaginationBar whenever there's any data (i.e. hide EuiPagination but keep showing itemsPerPagePopover). @chandlerprall what do you think?

To make sure we're all on the same page, this would look like this (again, this is what we were already doing)

chandlerprall · 2019-12-06T18:08:21Z

For user experience, I would prefer showing e.g. Page 1 of 1 instead of disappearing EuiPagination altogether.

thompsongl · 2019-12-06T18:14:35Z

Screenshots are helpful. Thanks!

I think that the itemsPerPagePopover text gives enough indication that there is no more data to be loaded, except in cases where the number of available rows equals the set number of rows per page. Adding 'Page 1 of 1' in place of EuiPagination as Chandler suggested seems like a good move for clarity.

andreadelrio · 2019-12-06T18:57:20Z

I think that the itemsPerPagePopover text gives enough indication that there is no more data to be loaded, except in cases where the number of available rows equals the set number of rows per page. Adding 'Page 1 of 1' in place of EuiPagination as Chandler suggested seems like a good move for clarity.

I think that itemsPerPagePopover is enough even when the number of available rows equals the chosen "rows per page" number. When seeing this, I would expect users to know that there's only one page.

Also, the caption that we will eventually add (#1237) could help make things even more clear for this particular case (# rows = chosen # of rows per page).

cchaos · 2019-12-06T19:34:29Z

@chandlerprall It sounds like you just don't agree with this ticket #899 to begin with?

The only problem with Page 1 of 1 is that's not what our pagination currently says, if that's explicitly what you want it to say.

I think that itemsPerPagePopover is enough even when the number of available rows equals the chosen "rows per page" number.

I actually think that is more confusing. The number of rows selector should not change (just like in your Mint example). It would be more indicative if you have only 3 rows and the selector says "10 rows per page" that you only have 3 results. Otherwise, I could possibly think there are more pages of 3 rows since I'm only seeing that same amount.

I think we've beaten this over the head too much. I would say that we should:

Only remove the pagination items if there is 0 data. (Not 0 search results)
Leave the rows per page number the same no matter how many results are available
Leave the page numbers even if there's only one page

andreadelrio · 2019-12-06T20:07:30Z

Going with Caroline's suggestions as I think they're a happy medium. Looks like this when there's only one page.

andreadelrio · 2019-12-06T20:14:22Z

I think that itemsPerPagePopover is enough even when the number of available rows equals the chosen "rows per page" number.

I actually think that is more confusing. The number of rows selector should not change (just like in your Mint example). It would be more indicative if you have only 3 rows and the selector says "10 rows per page" that you only have 3 results. Otherwise, I could possibly think there are more pages of 3 rows since I'm only seeing that same amount.

I think there might have been a misunderstanding, I wasn't suggesting the number of rows selector should change. I agree with the behaviour you're describing.

cchaos

I deleted my previous comment since I was a dummy and was looking at the published site.
The only scenario that still seems to be off is when searching in EuiMemoryTable and there are 0 results, I'd think we'd still want the whole pagination bar. But the empty message helps to convey that there aren't any results so is it necessary?

Other than that, just had some code cleanup comments.

cchaos · 2019-12-06T20:32:33Z

CHANGELOG.md

+- Hide `paginationBar` in `EuiBasicTable` when there is no data ([#2598](https://github.com/elastic/eui/pull/#2598))
+- Display `EuiPagination` in `EuiBasicTable` when there is only 1 page ([#2598](https://github.com/elastic/eui/pull/#2598))


These entries can be rolled into one because they both are affecting EuiBasicTable pagination options

cchaos · 2019-12-06T20:36:46Z

src/components/basic_table/basic_table.js

@@ -384,7 +384,7 @@ export class EuiBasicTable extends Component {
    );

    const table = this.renderTable();
-    const paginationBar = this.renderPaginationBar();
+    const paginationBar = this.renderPaginationBar(items);


A better thing to pass here would be the length of items so that there's an understanding of what the renderPaginationBar() method is doing with that parameter which is just checking that items to do exist.

Suggested change

const paginationBar = this.renderPaginationBar(items);

const paginationBar = this.renderPaginationBar(items.length);

cchaos · 2019-12-06T20:37:23Z

src/components/basic_table/basic_table.js

@@ -1049,9 +1049,9 @@ export class EuiBasicTable extends Component {
    return profile.align;
  }

-  renderPaginationBar() {
+  renderPaginationBar(items) {


Then here, you can change the name to something like

Suggested change

renderPaginationBar(items) {

renderPaginationBar(numItems) {

cchaos · 2019-12-06T20:39:06Z

src/components/pagination/pagination.tsx

@@ -173,7 +173,7 @@ export const EuiPagination: FunctionComponent<Props> = ({
    </EuiI18n>
  );

-  if (pages.length > 1) {
+  if (pages.length > 0) {


pages.length will never be less than 0 so this statement will always return true, which means that it's an unnecessary check the component should always return the full pagination and never an empty span.

andreadelrio · 2019-12-09T00:59:13Z

I deleted my previous comment since I was a dummy and was looking at the published site.
The only scenario that still seems to be off is when searching in EuiMemoryTable and there are 0 results, I'd think we'd still want the whole pagination bar. But the empty message helps to convey that there aren't any results so is it necessary?

I had missed the bit where you said "Not 0 search results" here => 1. Only remove the pagination items if there is 0 data. (Not 0 search results) I particularly think the message should be plenty to prevent the user from having any confusion with PaginationBar not showing up.

If we were to show PaginationBar when there are 0 search results it would look something like this:

I'm almost thinking that the user could find this buggy/weird. I associate "page number" with "there's some data" (regardless of if that data is "search results" or not).

chandlerprall · 2019-12-09T17:54:51Z

I'm happy with the direction this is going. I'll remove myself from the rest of the conversation, but do a final technical pass when the functionality is stable.

cchaos

I'm good with that decision (leaving pagination off if 0 results). LGTM!

chandlerprall

Changes LGTM; pulled & tested locally

Need to update &commit jest's snapshots for this to pass CI - yarn test-unit -u

andreadelrio · 2019-12-09T21:32:57Z

jenkins test this

andreadelrio added 4 commits December 5, 2019 11:11

hiding pagination when needed

dbc0655

Merge remote-tracking branch 'upstream/master' into rowsPerPage

a005546

hiding pagination when there are not enough rows

b55ef3f

cl

243a566

andreadelrio force-pushed the rowsPerPage branch from df1868e to 243a566 Compare December 6, 2019 03:59

andreadelrio requested review from cchaos and thompsongl December 6, 2019 04:08

andreadelrio added 3 commits December 6, 2019 12:01

show EuiPagination when there is 1 page

2bbd44d

Merge remote-tracking branch 'upstream/master' into rowsPerPage

4e6fdd8

update cl

664bf2b

andreadelrio changed the title ~~Display pagination in EuiBasicTable only when needed~~ Display paginationBar in EuiBasicTable only when needed Dec 6, 2019

andreadelrio changed the title ~~Display paginationBar in EuiBasicTable only when needed~~ Hide paginationBar in EuiBasicTable when there is no data Dec 6, 2019

extra line

d63690f

cchaos suggested changes Dec 6, 2019

View reviewed changes

cleanup

d98a56f

andreadelrio added 2 commits December 9, 2019 09:40

updated cl

0bc2686

fix ts error

60286e2

cchaos approved these changes Dec 9, 2019

View reviewed changes

chandlerprall approved these changes Dec 9, 2019

View reviewed changes

andreadelrio added 2 commits December 9, 2019 12:47

updated snaps

3d195a2

fix cl conflict

3616b1d

andreadelrio merged commit 6a193bc into elastic:master Dec 9, 2019

cqliu1 mentioned this pull request Feb 12, 2020

Infinite pagination in empty EuiBasicTable #2851

Closed

cchaos mentioned this pull request Feb 24, 2020

Fixes: #2851 : unnecessary pagination #2905

Closed

5 tasks

chandlerprall mentioned this pull request May 14, 2020

[EuiBasicTable] Move to next-to-last page if all items of last pages are deleted. #3431

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hide paginationBar in EuiBasicTable when there is no data #2598

Hide paginationBar in EuiBasicTable when there is no data #2598

andreadelrio commented Dec 6, 2019 •

edited

Loading

chandlerprall commented Dec 6, 2019 •

edited

Loading

chandlerprall commented Dec 6, 2019 •

edited

Loading

andreadelrio commented Dec 6, 2019

cchaos commented Dec 6, 2019

andreadelrio commented Dec 6, 2019

chandlerprall commented Dec 6, 2019

thompsongl commented Dec 6, 2019

andreadelrio commented Dec 6, 2019

cchaos commented Dec 6, 2019 •

edited

Loading

andreadelrio commented Dec 6, 2019

andreadelrio commented Dec 6, 2019

cchaos left a comment

cchaos Dec 6, 2019

cchaos Dec 6, 2019

cchaos Dec 6, 2019

cchaos Dec 6, 2019

andreadelrio commented Dec 9, 2019 •

edited

Loading

chandlerprall commented Dec 9, 2019

cchaos left a comment

chandlerprall left a comment

andreadelrio commented Dec 9, 2019

		- Hide `paginationBar` in `EuiBasicTable` when there is no data ([#2598](https://github.com/elastic/eui/pull/#2598))
		- Display `EuiPagination` in `EuiBasicTable` when there is only 1 page ([#2598](https://github.com/elastic/eui/pull/#2598))

	const paginationBar = this.renderPaginationBar(items);
	const paginationBar = this.renderPaginationBar(items.length);

Hide paginationBar in EuiBasicTable when there is no data #2598

Hide paginationBar in EuiBasicTable when there is no data #2598

Conversation

andreadelrio commented Dec 6, 2019 • edited Loading

Summary

Checklist

chandlerprall commented Dec 6, 2019 • edited Loading

chandlerprall commented Dec 6, 2019 • edited Loading

andreadelrio commented Dec 6, 2019

cchaos commented Dec 6, 2019

andreadelrio commented Dec 6, 2019

chandlerprall commented Dec 6, 2019

thompsongl commented Dec 6, 2019

andreadelrio commented Dec 6, 2019

cchaos commented Dec 6, 2019 • edited Loading

andreadelrio commented Dec 6, 2019

andreadelrio commented Dec 6, 2019

cchaos left a comment

Choose a reason for hiding this comment

cchaos Dec 6, 2019

Choose a reason for hiding this comment

cchaos Dec 6, 2019

Choose a reason for hiding this comment

cchaos Dec 6, 2019

Choose a reason for hiding this comment

cchaos Dec 6, 2019

Choose a reason for hiding this comment

andreadelrio commented Dec 9, 2019 • edited Loading

chandlerprall commented Dec 9, 2019

cchaos left a comment

Choose a reason for hiding this comment

chandlerprall left a comment

Choose a reason for hiding this comment

andreadelrio commented Dec 9, 2019

andreadelrio commented Dec 6, 2019 •

edited

Loading

chandlerprall commented Dec 6, 2019 •

edited

Loading

chandlerprall commented Dec 6, 2019 •

edited

Loading

cchaos commented Dec 6, 2019 •

edited

Loading

andreadelrio commented Dec 9, 2019 •

edited

Loading