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

[TSVB] Allows the user to change the tooltip mode #67775

Merged
merged 3 commits into from
Jun 9, 2020

Conversation

stratoula
Copy link
Contributor

@stratoula stratoula commented May 29, 2020

Summary

Fixes #60013. We added a dropdown to the panel options of TSVB in order the user to be able to toggle the tooltip mode. So now the user can also see in the tooltip only the values of the selected datapoint.

Screenshot 2020-05-29 at 6 03 43 PM

On the following video we can see the new functionality
Toggle_Tooltip_Mode_TSVB

Checklist

Delete any items that are not applicable to this PR.

@stratoula stratoula self-assigned this May 29, 2020
@stratoula stratoula marked this pull request as ready for review June 1, 2020 06:43
@stratoula stratoula requested a review from a team June 1, 2020 06:43
@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

@stratoula stratoula added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Jun 1, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@stratoula
Copy link
Contributor Author

stratoula commented Jun 1, 2020

We need help on the options of the dropdown menu from a writer.
Screenshot 2020-06-03 at 10 16 32 AM

@KOTungseth can you help me with this?

@stratoula stratoula requested a review from a team June 2, 2020 08:05
@stratoula stratoula force-pushed the add-tooltip-mode-switch branch 2 times, most recently from 1c236d5 to 23a6c86 Compare June 3, 2020 07:20
@KOTungseth
Copy link
Contributor

What do you think about changing Tooltip mode to Tooltip?

And for the dropdown, how about:

Show all values
Show single value

@stratoula
Copy link
Contributor Author

@KOTungseth I like it, it is much simpler

@stratoula stratoula requested a review from timroes June 4, 2020 08:59
@stratoula stratoula changed the title [TSVB] Allow the user to change the tooltip mode [TSVB] Allows the user to change the tooltip mode Jun 4, 2020
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.

Screenshots combined with text updates LGTM. 👍 Thanks!

Copy link
Contributor

@timroes timroes left a comment

Choose a reason for hiding this comment

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

We have a validation in place on the serverside, that validates all config sent to it. You need to add the tooltip_mode to the validation schema there, because currently this PR will log to the server console: Request validation error: [panels.0.tooltip_mode]: definition for this key is missing (saved object id: unsaved). This most likely means your TSVB visualization contains outdated configuration. You can report this problem under https://github.com/elastic/kibana/issues/new?template=Bug_report.md

@@ -247,6 +247,7 @@ export const visPayloadSchema = schema.object({
series: schema.arrayOf(seriesItems),
show_grid: numberIntegerRequired,
show_legend: numberIntegerRequired,
tooltip_mode: stringOptionalNullable,
Copy link
Contributor

@timroes timroes Jun 5, 2020

Choose a reason for hiding this comment

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

Instead of allowing any string here, could we please make this just specific for the options? schema.maybe(schema.oneOf(['show_all', 'show_focused']))

{
label: intl.formatMessage({
id: 'visTypeTimeseries.timeseries.tooltipOptions.showFocused',
defaultMessage: 'Show single value',
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 please rename this to Show focused values. Show single value does not really make sense, since we not really show only a single value, but if multiple values are close together we still show them all. I have the feeling it's rather confusing naming it "Show single value" and then actually showing multiple values in the tooltip.

Copy link
Contributor

@timroes timroes left a comment

Choose a reason for hiding this comment

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

Despite the label change, everything looks good to me. Tested on Firefox & Chrome Linux. Code LGTM. Once label changed and CI is green, feel free to merge

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@stratoula stratoula merged commit 88754d2 into elastic:master Jun 9, 2020
stratoula added a commit to stratoula/kibana that referenced this pull request Jun 9, 2020
* Allows the user to select the tooltip mode

* Fix problem on new TSVB and add new field to schema

* Change default value on tooltip dropdown for the focused series option
stratoula added a commit that referenced this pull request Jun 9, 2020
* Allows the user to select the tooltip mode

* Fix problem on new TSVB and add new field to schema

* Change default value on tooltip dropdown for the focused series option
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jun 9, 2020
* master: (37 commits)
  [DOCS] Adds documentation for drilldowns (elastic#68122)
  [Telemetry] telemetry.sendUsageFrom: "server" by default (elastic#68071)
  [ML] Transforms: Support sub-aggregations (elastic#68306)
  Fixed pre-configured docs link points to the wrong page and functional tests configs (elastic#68606)
  [Ingest Manager] Update queries from `stream.*` to `dataset.*` (elastic#68322)
  Enable Watcher by default to fix bug in which Watcher doesn't render in the side nav (elastic#68602)
  Convert Index Templates API routes to snakecase. (elastic#68463)
  [SECURITY SOLUTION] [Detections] Add / Update e2e tests to ensure initial rule runs are successful (elastic#68441)
  [Ingest] OpenAPI spec file (elastic#68323)
  chore(NA): skip apis Endpoint plugin Endpoint policy api (elastic#68638)
  bumping makelogs version to v6.0.0 (elastic#66163)
  [SIEM] Add create template button (elastic#66613)
  Bump websocket-extensions from 0.1.3 to 0.1.4 (elastic#68414)
  [ML] Sample data modules - use event.dataset instead of index name (elastic#68538)
  [ML] Functional tests - fix job validation API test with maxModelMemoryLimit (elastic#68501)
  [ML] Functional tests - stabilize DFA job creation (elastic#68495)
  [TSVB] Allows the user to change the tooltip mode (elastic#67775)
  chore(NA): skip apis Endpoint plugin Endpoint alert API when data is in elasticsearch (elastic#68613)
  chore(NA): skip endpoint Endpoint Alert Page: when es has data and user has navigated to the page (elastic#68596)
  [SIEM][Detection Engine] Converts from joi to use io-ts and moves the types to common  (elastic#68127)
  ...
@monfera
Copy link
Contributor

monfera commented Sep 7, 2020

Could it be added to the TSVB docs? The current one points to Style but doesn't seem to discuss the contents. See issue above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:TSVB TSVB (Time Series Visual Builder) release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TSVB should have an option to allow switching the tooltip mode
7 participants