refactor: decouple tooltip from XY chart #553

markov00 · 2020-02-14T14:15:49Z

Summary

This PR will decouple the tooltip from the XY chart to allow Partition and other chart type to use it.

BREAKING CHANGES
the SeriesIdentifier type is generalised into a simple object with two common values: specId and key. A specialised XYChartSeriesIdentifier extends now the base SeriesIdentifier
The SettingsSpec prop showLegendDisplayValue is renamed to showLegendExtra.
Previously showLegendDisplayValue was true by default always showing the legend extra
value. Now it becomes always hidden by default

close #246 this PR will hide the value by default, we will iterate over this to make this parameter customisable #561

Note for reviewers
It's easier to follow the changes one commit a time, as they are self contained.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

Any consumer-facing exports were added to src/index.ts (and stories only import from ../src except for test data & storybook)
This was checked for cross-browser compatibility, including a check against IE11
Proper documentation or storybook story was added for features that require explanation or tutorials
Unit tests were updated or added to match the most common scenarios
Each commit follows the convention

BREAKING CHANGE: the `SeriesIdentifier` type is generalized into a simple object with two common values: specId and key. A specialized `XYChartSeriesIdentifier` extends now the base `SeriesIdentifier`

The commit change the naming for few variables in the legend item object, removing some unnecessary code BREAKING CHANGE: The `SettingsSpec` prop `showLegendDisplayValue` is renamed to `showLegendExtra`

codecov-io · 2020-02-14T18:03:40Z

Codecov Report

Merging #553 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #553   +/-   ##
=======================================
  Coverage   71.59%   71.59%           
=======================================
  Files         212      212           
  Lines        6137     6137           
  Branches     1138     1138           
=======================================
  Hits         4394     4394           
  Misses       1724     1724           
  Partials       19       19

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ebb0637...886c636. Read the comment docs.

The isXValue is no longer required inside the TooltipValue object because we are already defining the header and the values inside two distinct objects

I've renamed the yAccessor field in favour of a more general valueAccessor. I've also added the seriesIdentifier to each tooltipValue

nickofthyme

I have a few comments to think about but otherwise looks good.

Tested locally, all the tooltip stories good fine to me.

src/components/tooltip/index.tsx

src/chart_types/xy_chart/utils/series.ts

nickofthyme · 2020-02-19T15:02:17Z

src/specs/settings.tsx

+  /**
+   * Show an extra parameter on each legend item defined by the chart type
+   */
+  showLegendExtra: boolean;


can we rename this to hideLegendExtra so the default is false? Then it would just be.

<Settings hideLegendExtra />

I think we should have the extra only visible if the user explicitly enable them (#246)

Ok in that case shouldn’t showLegendExtra default be false?

src/specs/settings.tsx

nickofthyme · 2020-02-19T15:06:46Z

src/chart_types/xy_chart/state/chart_state.test.ts

@@ -1028,21 +1034,27 @@ describe.skip('Chart Store', () => {
    store.annotationDimensions.set(rectAnnotationSpec.id, annotationDimensions);

    const highlightedTooltipValue: TooltipValue = {
-      name: 'foo',
+      label: 'foo',


Can we pick name or label to be consistent? I am having trouble choosing between the two. If we are going to have a name prop where this value is derived from, this should be name not label right?

Do you mean the name prop of the Series component?

Correct, this values is determined from the customSeriesLabel, soon to be name prop.

#539 (comment)

ok let me switch to name then

from our yesterday call, we will keep the label naming to make the tooltip a bit more abstract from the series. The Label should refer to the descriptive part of the value

change the default value of `showLegendExtra` to `false` BREAKING CHANGE: Previously `showLegendExtra` was true by default always showing the legend extra value. Now it becomes always hidden by default close elastic#246

This commit will decouple the tooltip component from the XY chart to allow Partition and other chart type an ease use. BREAKING CHANGE: the `SeriesIdentifier` type is generalized into a simplified object with two values in common: `specId` and `key`. A specialized `XYChartSeriesIdentifier` extends now the base `SeriesIdentifier`. The `SettingsSpec` prop `showLegendDisplayValue` is renamed to `showLegendExtra` and its default value is now `false` hiding the current/last value on the legend by default. close #246

# [18.0.0](v17.1.1...v18.0.0) (2020-03-17) ### Code Refactoring * clean up TS types ([#554](#554)) ([22f7635](22f7635)), closes [#547](#547) [#547](#547) * decouple tooltip from XY chart ([#553](#553)) ([e70792e](e70792e)), closes [#246](#246) ### Features * cleaner color API on SeriesSpec ([#571](#571)) ([f769f7c](f769f7c)) * **legend:** allow color picker component render prop ([#545](#545)) ([90f4b95](90f4b95)) * **partition:** add element click, over and out events ([#578](#578)) ([103df02](103df02)) * **partition:** add tooltip ([#544](#544)) ([6bf9a69](6bf9a69)), closes [#246](#246) * percentage display in partitioning charts ([#558](#558)) ([d6aa8d7](d6aa8d7)) * specify series name with a function on SeriesSpec ([#539](#539)) ([358455a](358455a)) * xAccessor can be a function accessor ([#574](#574)) ([bcc3d63](bcc3d63)) ### BREAKING CHANGES * The `getSpecId`, `getGroupId`, `getAxisId` and `getAnnotationId` are no longer available. Use a simple `string` instead. * `customSeriesColors` prop on `SeriesSpec` is now `color`. The `CustomSeriesColors` type is replaced with `SeriesColorAccessor`. * Remove `customSubSeriesName` prop on series specs in favor of cleaner api using just the `name` prop on `SeriesSpec`. The types `SeriesStringPredicate`, `SubSeriesStringPredicate` have been removed. * the `SeriesIdentifier` type is generalized into a simplified object with two values in common: `specId` and `key`. A specialized `XYChartSeriesIdentifier` extends now the base `SeriesIdentifier`. The `SettingsSpec` prop `showLegendDisplayValue` is renamed to `showLegendExtra` and its default value is now `false` hiding the current/last value on the legend by default.

# [18.0.0](elastic/elastic-charts@v17.1.1...v18.0.0) (2020-03-17) ### Code Refactoring * clean up TS types ([opensearch-project#554](elastic/elastic-charts#554)) ([6ce56fa](elastic/elastic-charts@6ce56fa)), closes [opensearch-project#547](elastic/elastic-charts#547) [opensearch-project#547](elastic/elastic-charts#547) * decouple tooltip from XY chart ([opensearch-project#553](elastic/elastic-charts#553)) ([cb02cd0](elastic/elastic-charts@cb02cd0)), closes [opensearch-project#246](elastic/elastic-charts#246) ### Features * cleaner color API on SeriesSpec ([opensearch-project#571](elastic/elastic-charts#571)) ([6a78c4e](elastic/elastic-charts@6a78c4e)) * **legend:** allow color picker component render prop ([opensearch-project#545](elastic/elastic-charts#545)) ([22ef1e6](elastic/elastic-charts@22ef1e6)) * **partition:** add element click, over and out events ([opensearch-project#578](elastic/elastic-charts#578)) ([4189573](elastic/elastic-charts@4189573)) * **partition:** add tooltip ([opensearch-project#544](elastic/elastic-charts#544)) ([0cffed4](elastic/elastic-charts@0cffed4)), closes [opensearch-project#246](elastic/elastic-charts#246) * percentage display in partitioning charts ([opensearch-project#558](elastic/elastic-charts#558)) ([993a448](elastic/elastic-charts@993a448)) * specify series name with a function on SeriesSpec ([opensearch-project#539](elastic/elastic-charts#539)) ([fc6430b](elastic/elastic-charts@fc6430b)) * xAccessor can be a function accessor ([opensearch-project#574](elastic/elastic-charts#574)) ([f702e2c](elastic/elastic-charts@f702e2c)) ### BREAKING CHANGES * The `getSpecId`, `getGroupId`, `getAxisId` and `getAnnotationId` are no longer available. Use a simple `string` instead. * `customSeriesColors` prop on `SeriesSpec` is now `color`. The `CustomSeriesColors` type is replaced with `SeriesColorAccessor`. * Remove `customSubSeriesName` prop on series specs in favor of cleaner api using just the `name` prop on `SeriesSpec`. The types `SeriesStringPredicate`, `SubSeriesStringPredicate` have been removed. * the `SeriesIdentifier` type is generalized into a simplified object with two values in common: `specId` and `key`. A specialized `XYChartSeriesIdentifier` extends now the base `SeriesIdentifier`. The `SettingsSpec` prop `showLegendDisplayValue` is renamed to `showLegendExtra` and its default value is now `false` hiding the current/last value on the legend by default.

markov00 added chore wip work in progress :tooltip Related to hover tooltip labels Feb 14, 2020

markov00 added 3 commits February 14, 2020 15:16

chore: remove unused interface

e8814e1

chore: remove unused interfaces

f3680e3

chore: move tooltips component to global folder

4344c73

markov00 force-pushed the 2020_02_13-decouple_tooltip branch from 0f9d96a to 4344c73 Compare February 14, 2020 14:17

markov00 added 3 commits February 14, 2020 15:29

chore: remove unused TooltipProps type

f56a0d3

refactor: generalize SeriesIdentifier

58def88

BREAKING CHANGE: the `SeriesIdentifier` type is generalized into a simple object with two common values: specId and key. A specialized `XYChartSeriesIdentifier` extends now the base `SeriesIdentifier`

refactor(legend): rename displayValue to extra

4a8c218

The commit change the naming for few variables in the legend item object, removing some unnecessary code BREAKING CHANGE: The `SettingsSpec` prop `showLegendDisplayValue` is renamed to `showLegendExtra`

markov00 added 9 commits February 14, 2020 19:17

refactor(tooltip): remove isXValue prop from TooltipValue

0c979eb

The isXValue is no longer required inside the TooltipValue object because we are already defining the header and the values inside two distinct objects

refactor: update TooltipValue type with decoupled names

2fa3fb0

I've renamed the yAccessor field in favour of a more general valueAccessor. I've also added the seriesIdentifier to each tooltipValue

refactor: decoupled tooltipHeaderFormatter selector

7c2d964

refactor: move annotation vis check inside tooltip vis check

0fd0042

refactor: move TooltipData type near to Tooltip component

6f34ca7

refactor: move isTooltipVisible to internal state

126532b

refactor: cleanup TooltipStateProps

ad1033a

refactor: move getTooltipValues in the internal state

e39fa4e

refactor: decouple tooltip anchor position

75448c8

markov00 removed the wip work in progress label Feb 18, 2020

markov00 marked this pull request as ready for review February 18, 2020 18:43

markov00 requested review from nickofthyme, monfera and rshen91 February 18, 2020 18:43

fix: rotated tooltip position

e1197ae

nickofthyme approved these changes Feb 19, 2020

View reviewed changes

markov00 added 2 commits February 20, 2020 12:21

refactor(legend): showLegendExtra to false by default

ebb0637

change the default value of `showLegendExtra` to `false` BREAKING CHANGE: Previously `showLegendExtra` was true by default always showing the legend extra value. Now it becomes always hidden by default close elastic#246

Merge branch 'master' into 2020_02_13-decouple_tooltip

886c636

markov00 changed the title ~~chore: decouple tooltip from XY chart~~ refactor: decouple tooltip from XY chart Feb 20, 2020

markov00 merged commit 1ed6957 into elastic:master Feb 20, 2020

markov00 deleted the 2020_02_13-decouple_tooltip branch February 20, 2020 13:53

markov00 mentioned this pull request Feb 26, 2020

Move the tooltip header out of tooltip values array #460

Closed

markov00 mentioned this pull request Mar 18, 2020

Update dependency @elastic/charts to v18.1.0 elastic/kibana#60578

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: decouple tooltip from XY chart #553

refactor: decouple tooltip from XY chart #553

markov00 commented Feb 14, 2020 •

edited

Loading

codecov-io commented Feb 14, 2020 •

edited

Loading

nickofthyme left a comment

nickofthyme Feb 19, 2020

markov00 Feb 19, 2020

nickofthyme Feb 19, 2020

nickofthyme Feb 19, 2020

markov00 Feb 19, 2020

nickofthyme Feb 19, 2020

markov00 Feb 19, 2020

markov00 Feb 20, 2020

refactor: decouple tooltip from XY chart #553

refactor: decouple tooltip from XY chart #553

Conversation

markov00 commented Feb 14, 2020 • edited Loading

Summary

Checklist

codecov-io commented Feb 14, 2020 • edited Loading

Codecov Report

nickofthyme left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markov00 commented Feb 14, 2020 •

edited

Loading

codecov-io commented Feb 14, 2020 •

edited

Loading