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

Fix interval drop-down for date histogram in discover #10384

Merged
merged 9 commits into from
Mar 28, 2017

Conversation

lukasolson
Copy link
Member

@lukasolson lukasolson commented Feb 15, 2017

When you first load discover for an index pattern with time-based events, you'll see a date histogram. Above the graph you'll see a link that shows you the selected interval (e.g. "12 hours"). When you click on the link, it turns into a drop-down that allows you to select a different interval.

Prior to this PR, the behavior was inconsistent. If you selected something other than "Auto", then the drop-down would persist. However, if you selected "Auto", it would disappear and turn into a link showing you what the automatically selected interval was. However, if you had already selected "Auto", then clicked on the interval (which would display the drop-down), then decided you wanted to keep "Auto", the only way to get back to showing the link with the interval would be to select a different interval and then select "Auto" again.

This PR changes this behavior so that the interval drop-down is always persistent, and doesn't switch from a link to a drop-down. In addition, if the selected interval results in having to scale the buckets, we show the scaled interval in a help text.

feb-28-2017 10-48-38

Fixes #6295.
Fixes #10351.

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.

Looks like a great improvement! I had one small suggestion you could implement, if you wanted.

@@ -102,6 +102,7 @@
class="form-control"
ng-model="state.interval"
ng-options="interval.val as interval.display for interval in intervalOptions | filter: intervalEnabled"
ng-blur="toggleInterval()"
Copy link
Contributor

Choose a reason for hiding this comment

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

One small suggestion... can we remove this function, and add two to replace it: showInterval and hideInterval? I think this explicitness clarifies the logic behind the behavior.

@lukasolson
Copy link
Member Author

After some discussion, we decided that it would probably be best to leave it always as a drop-down, and let the vis itself show the interval. We should probably use a similar presentation as the "Interval" drop-down when you're creating a vis:

feb-22-2017 12-36-03

Thoughts @cjcenizal @LeeDr @jimgoodwin?

@cjcenizal
Copy link
Contributor

Makes sense to de-duplicate this info. Originally, I didn't notice that this information was duplicated in the chart's X axis label.

@lukasolson
Copy link
Member Author

@cjcenizal @LeeDr Could you take another look at this now?

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.

Nice work! I'm not sure if the yellow "info" icon with the tooltip is the best way to notify the user that their selected range has been adjusted. I think we should make this a bit more obvious. Can we add the text "Scaled to 5 seconds" to make it clear right away what the scale is? The tooltip is still useful, in case the user wants to know why.

image

<kbn-info
ng-show="bucketInterval.scaled"
placement="right"
class="text-warning"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change this class to kuiIcon--info? It's a bit of a misuse of the class but I think it's OK in this instance because I doubt that class will change.

@lukasolson lukasolson changed the title Hide the time interval select on blur Fix interval drop-down for date histogram in discover Feb 28, 2017
@lukasolson
Copy link
Member Author

Stalled on #10607.

@lukasolson
Copy link
Member Author

jenkins test this

@lukasolson lukasolson removed the stalled label Mar 1, 2017
@lukasolson
Copy link
Member Author

I've merged master now that #10607 is fixed. @cjcenizal @LeeDr could you take one more look?

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.

LGTM!

@lukasolson lukasolson merged commit b11bbc4 into elastic:master Mar 28, 2017
lukasolson added a commit that referenced this pull request Mar 28, 2017
* Hide the time interval select on blur

* Show scale warning on discover and keep drop-down

* Show scale without need to hover over info icon

* Remove unused variables

* Fix discover page object tests

* Fix getChartInterval in functional test
@lukasolson
Copy link
Member Author

Backported to 5.x (5.4) in cd6a703.

@lukasolson lukasolson deleted the hide-discover-select-on-blur branch March 27, 2018 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants