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

Add new config option to control the amount of items in listing pages #11674

Merged
merged 3 commits into from
May 21, 2017
Merged

Add new config option to control the amount of items in listing pages #11674

merged 3 commits into from
May 21, 2017

Conversation

trevan
Copy link
Contributor

@trevan trevan commented May 9, 2017

Fixes #11415

This increases the default limit on the visualization and dashboard listing page to 1000 while also making it configurable.

The text is really rough and needs to be polished.

Add a warning message when the limit is reached.
@elasticmachine
Copy link
Contributor

Can one of the admins verify this patch?

@thomasneirynck
Copy link
Contributor

jenkins, test this

@thomasneirynck thomasneirynck added :Sharing review v6.0.0 Feature:Visualizations Generic visualization features (in case no more specific feature label is available) labels May 10, 2017
Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

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

thanks @trevan!

I think this is a useful shot-term solution until we get proper pagination for this.

@stacey-gammon, what do you think?

<div class="kuiInfoPanel kuiInfoPanel--warning">
<div class="kuiInfoPanelBody">
<div class="kuiInfoPanelBody__message">
The number of matching visualizations ({{ listingController.totalItems }}) is larger than the listing limit {{ listingController.listingLimit }}. Sorting will only work for the first {{ listingController.listingLimit }} visualizations. To see more, either filter the list or increase the config savedObjects:listingLimit.
Copy link
Contributor

@thomasneirynck thomasneirynck May 12, 2017

Choose a reason for hiding this comment

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

wrt

Sorting will only work for the first {{ listingController.listingLimit }} visualizations

imho, I would just strip this. i feel like the more detailed the error message is here, the more convoluted it gets for the reader. The first sentence and last are on the nose though.

@stacey-gammon
Copy link
Contributor

Code looks good, I second what @thomasneirynck said about the message though, I think shorter would be better.

Only thought I have is whether we should put it in the SavedObjectLoader directly, instead of just the dashboard and visualize landing pages, so saved object management can take advantage of it as well. If we do that though, I'd just want to be sure that increasing the limit to 1k doesn't affect the performance of the saved object management page, since it has all types of items listed in that table. Even so, I suspect it wouldn't be a problem.

But we can always investigate this as a second step, since at least this solves the problem for the landing pages.

@stacey-gammon
Copy link
Contributor

Actually, I took a closer look at the saved object management page and it has no pagination, it just lists the first 100 visualizations, so introducing it in there would be more work, so I'm fine with this PR going in as is, with just the listing pages using it.

I'm not 100% sure about the UI. Our pagers and the warnings are now inconsistent. With this PR it looks like this:
screen shot 2017-05-12 at 5 05 18 pm

While the other pager we have looks like this:
screen shot 2017-05-12 at 5 07 15 pm

The benefit of the first is that it's more informative, but the benefit of the second is that it's concise and subtle.

@cjcenizal @snide Any recommendations here?

@thomasneirynck
Copy link
Contributor

@stacey-gammon: fyi, we have another issue that tracks this similar issue for the Saved Object list: #8044

@trevan
Copy link
Contributor Author

trevan commented May 13, 2017

So, I'm trying to figure out what changes are wanted. It seams like I need to change the text (which I expected) but maybe you want me to have the text next to the pagination element? I'm fine with that change, though I would want this message to always be shown unlike with the discover tables. This is because the limit affects the table before you get to the end of the list because of sorting.

@stacey-gammon
Copy link
Contributor

@trevan - I'm waiting to see what feedback the design team (@cjcenizal @snide @alt74) comes back with regarding the placement of the error, error text, and when it should be shown, as they have the final say.

One idea I had for the sort warning was a small warning icon in the header row - maybe all the way to the right, or maybe in the column that is currently being sorted on (show the text on hover). And then maybe we can keep the 'limited results' warning to match the other table.

@cjcenizal
Copy link
Contributor

This design looks good to me. I'd refine the copy to be:

You have 201 visualizations, but your "listingLimit" setting prevents the table below from displaying more than 5. You can change this setting under Management > Advanced Settings.

@trevan
Copy link
Contributor Author

trevan commented May 19, 2017

@cjcenizal, is that the change I should make or should I wait for more input?

Copy link
Contributor

@stacey-gammon stacey-gammon left a comment

Choose a reason for hiding this comment

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

With the change in wording suggested by @cjcenizal, LGTM!

@cjcenizal
Copy link
Contributor

@trevan Thanks for checking. I think we're OK to move forward with this change.

@trevan
Copy link
Contributor Author

trevan commented May 20, 2017

I've updated the message per @cjcenizal comment above

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

Thanks for making that change @trevan! Sorry, I have one small request and then we can merge this.

<div class="kuiInfoPanel kuiInfoPanel--warning">
<div class="kuiInfoPanelBody">
<div class="kuiInfoPanelBody__message">
You have {{ listingController.totalItems }} dashboards, but your "listingLimit" setting prevents the table below from displaying more than {{ listingController.listingLimit }}. You can change this setting under Management &gt; Advanced Settings.
Copy link
Contributor

@cjcenizal cjcenizal May 20, 2017

Choose a reason for hiding this comment

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

I just realized we should probably just link directly to the Advanced Settings page. Can we change the last sentence to:

You can change this setting under <a kbn-href="#/management/kibana/settings" class="kuiLink">Advanced Settings</a>.

image

@cjcenizal
Copy link
Contributor

jenkins, test this

@trevan
Copy link
Contributor Author

trevan commented May 20, 2017

@cjcenizal, done

@cjcenizal
Copy link
Contributor

@trevan Thanks!

@cjcenizal
Copy link
Contributor

jenkins, test this

@cjcenizal cjcenizal merged commit 7557c8f into elastic:master May 21, 2017
cjcenizal pushed a commit to cjcenizal/kibana that referenced this pull request May 21, 2017
…elastic#11674)

* Add new config option to control the amount of items in listing pages
* Add a warning message when the limit is reached.
cjcenizal added a commit that referenced this pull request May 21, 2017
…#11674) (#11945)

* Add new config option to control the amount of items in listing pages
* Add a warning message when the limit is reached.
snide pushed a commit to snide/kibana that referenced this pull request May 30, 2017
…elastic#11674)

* Add new config option to control the amount of items in listing pages
* Add a warning message when the limit is reached.
@trevan trevan deleted the increase-listing-limit branch December 19, 2017 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Visualizations Generic visualization features (in case no more specific feature label is available) release_note:enhancement review v5.5.0 v6.0.0-alpha2 v6.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants