Skip to content

feat(tooltip): allow postfix banded area series#391

Merged
nickofthyme merged 6 commits intoelastic:masterfrom
nickofthyme:feat/upper-lower-band-label
Oct 2, 2019
Merged

feat(tooltip): allow postfix banded area series#391
nickofthyme merged 6 commits intoelastic:masterfrom
nickofthyme:feat/upper-lower-band-label

Conversation

@nickofthyme
Copy link
Copy Markdown
Collaborator

@nickofthyme nickofthyme commented Sep 24, 2019

Summary

Allow user to set postfix string or accessor for upper and lower bound of banded series to distinguish between values.

Adds y1AccessorFormat and y0AccessorFormat props to BasicSeriesSpec. y1AccessorFormat defaults to ' - upper' and y0AccessorFormat defaults to ' - lower'

AccessorFormat = string | ((value: string) => string);

closes #162

see local storybook link http://localhost:9001/?path=/story/area-chart--band-area-chart

Area banded series

Screen Recording 2019-09-24 at 12 17 PM

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

Allow user to set postfix for upper and lower bound of banded series to distinguish between values

closes elastic#162
@nickofthyme nickofthyme added the :data Data/series/scales related issue label Sep 24, 2019
@codecov-io
Copy link
Copy Markdown

codecov-io commented Sep 24, 2019

Codecov Report

Merging #391 into master will decrease coverage by 0.2%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #391      +/-   ##
==========================================
- Coverage   98.25%   98.05%   -0.21%     
==========================================
  Files          38       39       +1     
  Lines        2814     2833      +19     
  Branches      666      673       +7     
==========================================
+ Hits         2765     2778      +13     
- Misses         44       50       +6     
  Partials        5        5
Impacted Files Coverage Δ
src/chart_types/xy_chart/store/utils.ts 96.7% <ø> (ø) ⬆️
src/utils/accessor.ts 45.45% <100%> (ø)
src/chart_types/xy_chart/tooltip/tooltip.ts 93.22% <100%> (+0.36%) ⬆️
src/chart_types/xy_chart/rendering/rendering.ts 98.15% <100%> (+0.03%) ⬆️
src/chart_types/xy_chart/utils/specs.ts 100% <100%> (ø) ⬆️

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 0f587d0...7304cb3. Read the comment docs.

@nickofthyme nickofthyme marked this pull request as ready for review September 27, 2019 18:59
@nickofthyme nickofthyme mentioned this pull request Sep 28, 2019
5 tasks
Copy link
Copy Markdown
Collaborator

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

I've left a suggestion to consider before merging this.
I've tested this locally and works fine.

*
* @default 'lower'
*/
y1AccessorPostfix?: string;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hey @nickofthyme
can we change this to a postfix string OR a function that is invoked with the series name and return the user formatted series name like:

y0AccessorName={(seriesName) => { return `[min] ${seriesName}`; }}

because I think the position of the min/max fix depends on the user's language.
We can support both a string or a function?
In the case of the function, we can use it also on non-banded situations?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Dangit! You're right, I should have thought of that.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@nickofthyme nickofthyme requested a review from markov00 October 1, 2019 13:53
Copy link
Copy Markdown
Collaborator

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

LGTM

}

if (banded) {
if (y0Accessors && y0Accessors.length > 0) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: small utility function?
as we are using this also on src/chart_types/xy_chart/store/utils.ts?

hasY0Accessors({y0Accessors}) {
    return y0Accessors && y0Accessors.length > 0
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@nickofthyme nickofthyme merged commit dfd5d7b into elastic:master Oct 2, 2019
@nickofthyme nickofthyme deleted the feat/upper-lower-band-label branch October 2, 2019 14:47
markov00 pushed a commit that referenced this pull request Oct 2, 2019
# [13.3.0](v13.2.0...v13.3.0) (2019-10-02)

### Features

* **tooltip:** tooltip label format for upper/lower banded area series ([#391](#391)) ([dfd5d7b](dfd5d7b)), closes [#162](#162)
@markov00
Copy link
Copy Markdown
Collaborator

markov00 commented Oct 2, 2019

🎉 This PR is included in version 13.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Oct 2, 2019
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
# [13.3.0](elastic/elastic-charts@v13.2.0...v13.3.0) (2019-10-02)

### Features

* **tooltip:** tooltip label format for upper/lower banded area series ([opensearch-project#391](elastic/elastic-charts#391)) ([701264c](elastic/elastic-charts@701264c)), closes [opensearch-project#162](elastic/elastic-charts#162)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:data Data/series/scales related issue released Issue released publicly

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Display y0 and y1 names on tooltip for band charts

3 participants