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

Adds ability to hide per page options when enabling table pagination #1130

Merged
merged 4 commits into from
Aug 22, 2018

Conversation

jasonrhodes
Copy link
Member

@jasonrhodes jasonrhodes commented Aug 21, 2018

On the APM team we are hoping to be able to hide the "per page" select box in table pagination, while still leaving the right-aligned page navigation buttons. (The default would be to show those options, which leaves everything working as-is.)

In master:
screen shot 2018-08-21 at 3 41 49 pm

vs in this PR:
screen shot 2018-08-21 at 3 42 07 pm

This PR addresses a few things:

  • Adds a new property on the pagination object, called disablePerPageOptions
  • The underlying EuiTablePagination component accepts this property and, when true, renders null in the left-aligned EuiFlexItem where that select box usually shows up, which keeps the rest of the layout in place
  • EuiBasicTable also accepts this same property on its pagination object, and passes it along
  • EuiInMemoryTable accepts the same property on its pagination object, and also passes it along (with some extra work since that object gets transformed a bit on the way in)
  • Example docs for these tables are updated to include the property, where props are defined, and to show a toggle button that changes that prop to show what the table looks like with and without the per page options select box, without having to create a whole new table demo

Would be happy to jump on a zoom to discuss, talk through some other options, change property names or improve on the docs, etc. Thanks!

@jasonrhodes
Copy link
Member Author

Note: this addresses a todo item from elastic/kibana#21300

@jasonrhodes
Copy link
Member Author

retest this please

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.

Not sure I understand why you would ever want to hide this functionality from the user... ;) But, it works as describes.

Just had a couple docs comments and one about naming.

@@ -104,4 +109,5 @@ EuiTablePagination.propTypes = {
EuiTablePagination.defaultProps = {
itemsPerPage: 50,
itemsPerPageOptions: [10, 20, 50, 100],
disablePerPageOptions: false
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this prop should be renamed to hidePerPageOptions as the word "disable" seems to imply that it will still exist but just be grayed out and non-interactive.

Also, please add it to the EuiTablePagination.propTypes object as well.

onChange={this.onTableChange}
/>
<EuiSpacer size="xl" />
<EuiSwitch
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this switch to before the table to coincide with the other examples.

@@ -83,7 +83,7 @@ export const Table = () => {
<EuiInMemoryTable
items={store.users}
columns={columns}
pagination={true}
pagination={{ disablePerPageOptions: true }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep the pagination option in this example. But, again, good to know it works with custom tables.

<EuiSpacer size="xl" />

<EuiSwitch
label={<span>Hide per page options with <EuiCode>pagination.disablePerPageOptions = true</EuiCode></span>}
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you already have this toggle to show the functionality in the pagination example, we don't need it again in the custom table. (Good to know it works with this type of table)

Copy link
Member Author

Choose a reason for hiding this comment

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

This was the one place I think it'd be good to have something in addition to the toggle in the basic table's pagination example, just to make sure it's clear that you can pass this option to the basic table or to the EuiTablePagination component used on its own.

I removed the switch toggle and added EuiTablePagination to the list of props for this custom table and I think that satisfies it, but let me know what you think.

@jasonrhodes
Copy link
Member Author

Changes addressed in ca9fb24 and 143aafe

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.

LGTM, But it'd be good to also get an engineer's eyes on it as well.

Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

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

Nothing to add. LGTM

@sorenlouv
Copy link
Member

Thanks for doing this @jasonrhodes !

@jasonrhodes jasonrhodes merged commit dd413d5 into elastic:master Aug 22, 2018
@jasonrhodes jasonrhodes deleted the hide-per-page-options branch August 22, 2018 18:18
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