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

feat(goal): add valueFormatter for tooltip #1529

Merged
merged 13 commits into from
Jan 5, 2022

Conversation

rshen91
Copy link
Contributor

@rshen91 rshen91 commented Dec 17, 2021

Summary

The tooltip for the goal charts will now accept a valueFormatter prop for the tooltip.

...
valueFormatter={(d: number) => `Testing ${d}`}
...

Screen Shot 2021-12-27 at 8 25 14 AM

Details

  • Need to figure out why CentralMajor parameter isn't respecting $ (needed two $s to display the $ in the story)

Issues

Closes #1521

Checklist

  • The proper chart type label has been added (e.g. :xy, :partition)
  • The proper feature labels have been added (e.g. :interactions, :axis)
  • All related issues have been linked (i.e. closes #123, fixes #123)
  • New public API exports have been added to packages/charts/src/index.ts
  • Unit tests have been added or updated to match the most common scenarios

@rshen91 rshen91 added :goal/gauge (old) Old Goal/Gauge chart related issues :interactions Interactions related issue labels Dec 27, 2021
@@ -47,7 +47,7 @@ export const getTooltipInfoSelector = createCustomCachedSelector(
key: spec.id,
},
value,
formattedValue: `${value}`,
formattedValue: valueFormatter ? valueFormatter(value) : `${value}`,
Copy link
Member

Choose a reason for hiding this comment

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

From your typings valueFormatter is always available, no need to check for its existence.

Suggested change
formattedValue: valueFormatter ? valueFormatter(value) : `${value}`,
formattedValue: spec.valueFormatter(value),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok that makes sense - d41cb16 for changes thanks

@@ -80,6 +82,7 @@ export const defaultGoalSpec = {
bandLabels: [],
angleStart: Math.PI + Math.PI / 4,
angleEnd: -Math.PI / 4,
valueFormatter: (value: any) => value,
Copy link
Member

Choose a reason for hiding this comment

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

please avoid the usage of any. ValueFormatter actually is a function with well known type as argument

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Smart - d41cb16 for changes

@@ -74,6 +76,7 @@ export const Example = () => {
centralMinor=""
angleStart={angleStart}
angleEnd={angleEnd}
valueFormatter={useValueFormatter ? (d: any) => `Testing ${d}` : undefined}
Copy link
Member

Choose a reason for hiding this comment

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

You can probably just issue a nice valueFormatter that reflects the chart context like

valueFormatter={(d) => `${d}k USD`}

and avoid the knob

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you d41cb16 for changes

@rshen91 rshen91 marked this pull request as ready for review December 27, 2021 13:46
@rshen91 rshen91 requested a review from markov00 December 27, 2021 14:55
Copy link
Collaborator

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

Looks good, had a few questions.

storybook/stories/goal/2_gauge_with_target.story.tsx Outdated Show resolved Hide resolved
packages/charts/src/chart_types/goal_chart/specs/index.ts Outdated Show resolved Hide resolved
packages/charts/src/chart_types/goal_chart/specs/index.ts Outdated Show resolved Hide resolved
packages/charts/src/components/accessibility/types.tsx Outdated Show resolved Hide resolved
@rshen91 rshen91 requested a review from nickofthyme January 5, 2022 14:33
@rshen91 rshen91 merged commit 8139973 into elastic:master Jan 5, 2022
@rshen91 rshen91 deleted the gauge-tooltip branch January 5, 2022 15:19
nickofthyme pushed a commit that referenced this pull request Jan 5, 2022
# [42.0.0](v41.0.1...v42.0.0) (2022-01-05)

### Bug Fixes

* **flamegraph:** solve animation regression occurring with 6db2677 ([#1541](#1541)) ([5ec6037](5ec6037)), closes [#1540](#1540)
* **heatmap:** render empty state ([#1532](#1532)) ([59002df](59002df))
* **waffle:** fix strange 0 text in legend item extra when label is 0 ([#1538](#1538)) ([72224b9](72224b9))

### Features

* **goal:** add valueFormatter for tooltip ([#1529](#1529)) ([8139973](8139973))
* **heatmap:** add axis titles ([#1503](#1503)) ([a87325d](a87325d))
* **types:** improve generic types in specs, and spec prop types ([#1421](#1421)) ([562929e](562929e))

### BREAKING CHANGES

* **types:** The `xAccessor` and `yAccessor` are now required on all xy chart specs. Stronger typing on `data` prop that may cause type errors when using untyped array (i.e. `const arr: never[] = []`). Other minor type changes related to spec types.
* **heatmap:** The heatmap yAxisLabel.padding style type is changed from Pixel | Partial<Padding> to Pixels | Padding. The heatmap axis labels are now correctly subjected to padding calculations and it will result in a slightly different position of labels.

Co-authored-by: Marco Vettorello <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:goal/gauge (old) Old Goal/Gauge chart related issues :interactions Interactions related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Gauge] allow the tooltip value to be formatted
3 participants