Show/hide date histogram viz in Discover #17065

ycombinator · 2018-03-09T14:09:06Z

Resolves #6164.

This PR introduces an Advanced Settings option to show/hide the date histogram visualization in Discover.

The option is named discover:showDateHistogramVisualization. It is a boolean option that defaults to true. Setting it to false removes the aggs part of the _msearch request that is made by Discover and hides the time chart panel in Discover.

Default behavior

When discover:showDateHistogramVisualization is set to true. _msearch request contains aggs and the visualization is shown in Discover.

Disabled behavior

When discover:showDateHistogramVisualization is set to false. _msearch request does not contain aggs and the visualization is not shown in Discover.

elasticmachine · 2018-03-09T14:51:49Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-03-09T17:24:37Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

lukasolson

I'm all for allowing the user the flexibility to remove the histogram, but I think a better way to do this that is more in line with how we are already doing things like this is similar to how you can collapse the sidebar and hide the fields list:

Long term, we'd like the ability for Discover to be more customizable and allow any of the components to be removed and others to be added. I think what I've explained above is a better short-term solution than adding an advanced setting that we will likely remove in the future.

ycombinator · 2018-03-09T20:33:00Z

Thanks for the feedback @lukasolson. I considered your suggestion as well and I'm ++ for it. It definitely feels more "natural" as a user (than having to edit an Advanced Setting, which would also have the effect of being applicable to all users on that Kibana instance).

There's just one UX issue that I couldn't reason out which is why I went the Advanced Setting route: if we introduce a toggle in the Discover UI itself, where would you store its state? Would it be per user session (i.e. using something like sessionStorage)? Or would you prefer per browser (i.e. using localStorage) so it persists even if the same browser is reopened? Or were you thinking something else?

I'm also completely okay just abandoning this PR altogether if there's a longer-term plan for Discover that would make this functionality moot.

Bargs · 2018-03-12T13:15:21Z

Do we have a good sense of why people are requesting this feature? I know some folks want to get rid of it because the date histogram can be slow, but the ES team is working on this. Is there any other reason why an admin would want to disable the histogram on a system wide level?

If some folks simply find the histogram annoying and want to regain the real estate it takes up, I think localstorage would be fine, similar to the left nav's collapse/expand.

gsemet · 2018-03-13T17:37:56Z

Hello. Please expose this ability by an URL parameter. That is enough for me

ycombinator · 2018-03-24T15:06:19Z

Based on the above feedback (thanks for giving it!) and chatting with @Bargs off-PR, I'm thinking of changing this PR to:

provide an expand/collapse toggle in the UI (per @lukasolson's suggestion)
maintain the toggle state in appState (@Bargs suggestion off-PR), which gets serialized in the URL (satisfying @gsemet's request).

LeeDr · 2018-03-29T20:54:40Z

I just did a little experiment to try to achieve this without any code changes. I just created a similar index pattern for my logstash data but selected I don't want to use a time filter. So now in Discover I can toggle between the 2 index patterns. One has the histogram and the other doesn't. But the one that doesn't have the time field also doesn't have the time picker so you just get the first 500 docs and would have to manually filter on whatever time you wanted to see. Not great, but could be a work-around for someone until this gets implemented and released.

…very The option is named `discover:showDateHistogramVisualization`. It is a boolean option that defaults to `true`. Setting it to `false` removes the `aggs` part of the `_msearch` request that is made by Discover and hides the time chart panel in Discover.

elasticmachine · 2018-04-20T17:33:58Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

ycombinator · 2018-04-20T18:10:46Z

@lukasolson @Bargs @gsemet I've updated this PR per #17065 (comment):

The gif doesn't show it but if you pull down this PR and try it out yourself you'll see that showViz is also changing in the app state encoded in the URL.

Once ya'll are happy with the implementation, I'll add functional tests to this PR.

gsemet · 2018-04-20T18:11:55Z

Looks amazing :)

elasticmachine · 2018-04-20T18:34:03Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

lukasolson · 2018-04-23T21:34:24Z

@snide Any feedback here on the UI?

snide · 2018-04-23T21:50:13Z

It's fine. I can clean it up separately. But for now it's similar and works!

lukasolson

Noticed some funky behavior...

Hide the viz
Refresh
Try to show the viz (it only shows part of it)

Also, I was thinking, with this change we would have three different portions of the UI you can show/hide: The global nav, which is stored in local storage (kibana.isGlobalNavOpen), show/hide the viz, which is stored in the app state, and show/hide the field list, which isn't stored anywhere. Not sure how we should reconcile this, but wanted to open the dialog.

lukasolson

I'll leave my general feedback here: Personally I think it'd be nice if showViz were stored in local storage because every time you load up discover, you're going to get that viz at first until you hide it, which might be okay but I wonder if some people just don't ever want to see it there and will be annoyed that they have to hide it every time they reload discover, or load a new saved search, etc.

That being said, what we've got here is definitely an improvement and I don't want to block it from moving forward, and we can have the discussion at a later time.

ycombinator · 2018-04-24T19:50:13Z

@lukasolson the reason this ended up as a URL parameter was because @gsemet requested it. But your point about the initial state of Discover is a good one. @gsemet, would it work for you if we stored the state in local storage instead of the URL?

elasticmachine · 2018-04-24T20:18:20Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

Bargs

One largish question: If the visualization is hidden, should we remove the agg from the search request? Some users definitely want to get rid of the date histogram because it can slow Discover down. It would complicate the code slightly because we'd need to re-fetch when the viz is made visible, but it might be worth it to improve the user experience.

Bargs · 2018-04-30T18:08:56Z

src/core_plugins/kibana/public/discover/index.html

@@ -139,6 +146,13 @@ <h2>Searching</h2>
                 style="height: 200px"
                >
              </visualization>
+
+              <div>
+                  <center class="small">


center is a deprecated html tag. I'm guessing you're using it because it's already used in the vis header. Since it's only used in those two places, I think it's worth fixing. It looks like they both only contain inline elements, so you should be able to just change it to a div and add text-align:center;

Bargs · 2018-04-30T18:20:19Z

src/core_plugins/kibana/public/discover/controllers/discover.js

@@ -292,6 +293,8 @@ function discoverController(
    $state.save();
  });

+  $scope.$watch('state.isVisualizationVisible', $state.save.bind($state));


Couldn't this go in the showVis and hideVis methods? Then we wouldn't have to create an additional watch.

ycombinator · 2018-05-04T10:46:08Z

One largish question: If the visualization is hidden, should we remove the agg from the search request? Some users definitely want to get rid of the date histogram because it can slow Discover down. It would complicate the code slightly because we'd need to re-fetch when the viz is made visible, but it might be worth it to improve the user experience.

Yup, this is worth doing. I'll look into it.

rashmivkulkarni · 2018-07-17T04:43:21Z

@ycombinator - FF for 6.3.2 is tomorrow ( July 17) and this has been labelled for 6.3.2 ...Do you think we should remove the label and assign appropriately? Thanks.

spalger · 2019-09-19T18:34:18Z

This PR doesn't contain code necessary for it to run on CI with Jenkins pipelines, please merge the base branch in before removing the test-matrix label.

elasticmachine · 2019-09-19T18:35:02Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

LeeDr · 2019-09-19T21:18:59Z

@ycombinator if I open Discover and initially have the histogram, then I click to hide it. I would assume this would NOT re-query at this time because it's not going to change the data the user sees?

And of course going the other way to WOULD re-query automatically.

ycombinator · 2019-09-19T21:24:33Z

@LeeDr I'm honestly not sure any more. This PR is over 18 months old. I'm tempted to just close it unmerged and let someone better-versed than me in the Kibana codebase today find a solution. Thoughts?

lukasolson · 2020-03-25T22:52:51Z

Closing for inactivity.

ycombinator added release_note:enhancement review :Discovery v7.0.0 v6.3.0 labels Mar 9, 2018

ycombinator requested review from lukasolson and Bargs March 9, 2018 14:09

ycombinator changed the title ~~Add Advanced Settings option to show/hide date histogram viz in Discover~~ Show/hide date histogram viz in Discover via Advanced Setting Mar 9, 2018

ycombinator force-pushed the enhancement/discovery/hide-chart branch from 4fbb877 to e5a13a9 Compare March 9, 2018 16:39

lukasolson reviewed Mar 9, 2018

View reviewed changes

ycombinator force-pushed the enhancement/discovery/hide-chart branch from e5a13a9 to 9d5aea8 Compare April 20, 2018 17:01

ycombinator added 3 commits April 20, 2018 10:58

Add show/hide viz link + flag in app state

bc62c4c

Remove flag from Advanced Options

3a280f3

Using .bind instead of =>

4156c7e

ycombinator changed the title ~~Show/hide date histogram viz in Discover via Advanced Setting~~ Show/hide date histogram viz in Discover Apr 20, 2018

lukasolson requested changes Apr 23, 2018

View reviewed changes

lukasolson reviewed Apr 24, 2018

View reviewed changes

Bargs suggested changes Apr 30, 2018

View reviewed changes

jgough mentioned this pull request May 8, 2018

Result streaming on the discover tab #15662

Closed

rayafratkina added v6.3.1 and removed v6.3.0 labels Jun 13, 2018

jinmu03 added v6.3.2 and removed v6.3.1 labels Jul 5, 2018

tylersmalley added v6.4.0 and removed v6.3.2 labels Jul 17, 2018

LeeDr added v6.5.0 and removed v6.4.0 labels Jul 25, 2018

ycombinator removed the v6.5.0 label Oct 14, 2018

snide added v7.0.1 and removed v7.0.0 labels Apr 10, 2019

rayafratkina added Feature:Discover Discover Application Team:Visualizations Visualization editors, elastic-charts and infrastructure and removed Feature:Discovery v7.0.1 labels Jun 14, 2019

spalger added the test-matrix Use this label to ensure PRs are tested with matrix jobs label Sep 19, 2019

lukasolson closed this Mar 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show/hide date histogram viz in Discover #17065

Show/hide date histogram viz in Discover #17065

ycombinator commented Mar 9, 2018 •

edited

Loading

elasticmachine commented Mar 9, 2018

elasticmachine commented Mar 9, 2018

lukasolson left a comment

ycombinator commented Mar 9, 2018

Bargs commented Mar 12, 2018

gsemet commented Mar 13, 2018 •

edited

Loading

ycombinator commented Mar 24, 2018

LeeDr commented Mar 29, 2018

elasticmachine commented Apr 20, 2018

ycombinator commented Apr 20, 2018

gsemet commented Apr 20, 2018

elasticmachine commented Apr 20, 2018

lukasolson commented Apr 23, 2018

snide commented Apr 23, 2018

lukasolson left a comment

lukasolson left a comment

ycombinator commented Apr 24, 2018

elasticmachine commented Apr 24, 2018

Bargs left a comment

Bargs Apr 30, 2018

Bargs Apr 30, 2018

ycombinator commented May 4, 2018

rashmivkulkarni commented Jul 17, 2018

spalger commented Sep 19, 2019

elasticmachine commented Sep 19, 2019

LeeDr commented Sep 19, 2019

ycombinator commented Sep 19, 2019

lukasolson commented Mar 25, 2020

Show/hide date histogram viz in Discover #17065

Show/hide date histogram viz in Discover #17065

Conversation

ycombinator commented Mar 9, 2018 • edited Loading

Default behavior

Disabled behavior

elasticmachine commented Mar 9, 2018

💔 Build Failed

elasticmachine commented Mar 9, 2018

💚 Build Succeeded

lukasolson left a comment

Choose a reason for hiding this comment

ycombinator commented Mar 9, 2018

Bargs commented Mar 12, 2018

gsemet commented Mar 13, 2018 • edited Loading

ycombinator commented Mar 24, 2018

LeeDr commented Mar 29, 2018

elasticmachine commented Apr 20, 2018

💔 Build Failed

ycombinator commented Apr 20, 2018

gsemet commented Apr 20, 2018

elasticmachine commented Apr 20, 2018

💔 Build Failed

lukasolson commented Apr 23, 2018

snide commented Apr 23, 2018

lukasolson left a comment

Choose a reason for hiding this comment

lukasolson left a comment

Choose a reason for hiding this comment

ycombinator commented Apr 24, 2018

elasticmachine commented Apr 24, 2018

💔 Build Failed

Bargs left a comment

Choose a reason for hiding this comment

Bargs Apr 30, 2018

Choose a reason for hiding this comment

Bargs Apr 30, 2018

Choose a reason for hiding this comment

ycombinator commented May 4, 2018

rashmivkulkarni commented Jul 17, 2018

spalger commented Sep 19, 2019

elasticmachine commented Sep 19, 2019

💚 Build Succeeded

LeeDr commented Sep 19, 2019

ycombinator commented Sep 19, 2019

lukasolson commented Mar 25, 2020

ycombinator commented Mar 9, 2018 •

edited

Loading

gsemet commented Mar 13, 2018 •

edited

Loading