[Discover] Add "Chart options" menu

[kertal]

Summary

This PR implements the "Charts options" menu in the top section of the main area of Discover. With the upcoming ML's Data Visualizer this area gets crowded (it was already, especially in mobile view). So now it looks like this:

Hiding the chart and assigning it's interval were merged into this new element

The warning that the selected interval was invalid is also inside the Chart options menu (Usually selecting Milliseconds as interval with a long time range triggers this warning):

Additionally the "Count" label of the y-axis was removed (since it's obvious what's displayed)

Fixes #112149

Checklist

Delete any items that are not applicable to this PR.

• Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
• Unit or functional tests were updated or added to match the most common scenarios
• Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
• Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
• This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
• This was checked for cross-browser compatibility

[elasticmachine]

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

[kertal]

dear @gchaps , just a question about wording, is "Chart options" fine? thx ... ah yes, also a ping, because this will be a change to document :)

[timroes]

In general code looks good to me, I would suggest two more things:

In the original ticket and meeting we've discussed moving the "time range" (currently above the chart) into the x-axis label of the chart. That way we get that space free for the view switcher. I think we should still do that within this PR. There is currently a tooltip assigned stating "To change the time, use the global time filter." I don't think we need to cary that over to the axis legend (if that would even be possible).

Moving that over also has the nice advantage we can improve the movile view. Currently it looks like this:

I think it would be better if the date is in the x-axis label and instead we could keep the hit count left aligned and the chart setting right aligned within the same row.

[kertal]

@timroes yes, this looks much better, however, the x-axis is a bit long, and can be truncated depending on window width

So I wonder, is info like "order_date per 30 seconds" really necessary?

[kertal]

@elasticmachine merge upstream

[andreadelrio]

I think this is looking nice! Just a couple of nits since we're already in there:

1. I noticed there's more space between the chart and the x-axis label than between the x-axis label and the table. Can we reduce the former?

2. There's also unequal white space on the sides of the chart. This one might be trickier to solve but at the very least I would ask that we set padding-right to zero for .dscHistogram

Also noticed that the x-axis label is not wrapping in small screens.

[timroes]

@kertal I agree, I think we should just get rid of the actual field name and interval and only have the timestamp there, nothing else.

If we are still worried about them being cut off on smaller screens then, I think what we could use the useIsWithinBreakpoints method from EUI, to shorten or hide that label completely on small screens.

[kertal]

@kertal I agree, I think we should just get rid of the actual field name and interval and only have the timestamp there, nothing else.

If we are still worried about them being cut off on smaller screens then, I think what we could use the useIsWithinBreakpoints method from EUI, to shorten or hide that label completely on small screens.

@timroes think this is fine, also for smaller screens:

@andreadelrio note that we should keep padding of .dscHistogram, since this whitespace is used by the legend. I've added bottom padding to the chart, it was set to 0 (@markov00 can I reduce the distance of the x-axis label to the axis of Discover histogram ? Thx!)

[kertal]

@elasticmachine merge upstream

[timroes]

Tested on Chrome Linux. Seems to work finde. Code LGTM assuming green CI.

I think there is just some minor change we need to be aware of, but I think they are all fine:

When you hide the chart you now no longer see the timerange, since it's hidden too. I think that is okay, and we need that original space still for the view switcher later.

When showing a chart again, the shown time range is calculated again. That is currently fine, since we anyway do a full new request when showing the chart. If we'd just load the chart data again when showing the chart again, and not the documents, we'd still keep updating the shown time range. Though in that case we'd anyway show the chart on a different time range than the documents, if we'd implement that, and the quesiton would anyway be: what should that timerange represent. Thus I am also fine with that behavior.

[andreadelrio]

Seems like the label is getting cut off in mobile.

[kertal]

Seems like the label is getting cut off in mobile.

@andreadelrio Yes, but I don't think there's a good way to solve this, since it's the label of the x-axis. What we could do is rendering those outside the canvas, then we would have all options

[kertal]

@andreadelrio @timroes so, that's how it could look like, when showing the time range outside the canvas
👍 not cut of in mobile
👎 needs more vertical screen space
Applied it to this PR, easy to go back

[andreadelrio]

@andreadelrio @timroes so, that's how it could look like, when showing the time range outside the canvas
👍 not cut of in mobile
👎 needs more vertical screen space

@kertal thanks for exploring that route. I'd add negative margin-top to .dscHistogramTimeRange and I'd say we're good to go.

.dscHistogramTimeRange {
   margin-top: - $euiSizeS;
}

[andreadelrio]

LGTM!

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 22dfcd0
• Storybooks Preview
• Documentation Changes

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
discover	437	427	-10

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
discover	378.7KB	377.7KB	-1006.0B

History

• 💚 Build #155804 succeeded 91fb484
• 💔 Build #155715 failed 518ca3f
• 💔 Build #155681 failed 9c23ed2
• 💔 Build #155459 failed 0d616fd
• 💔 Build #155322 failed 7059cff

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @kertal