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(tooltip): allow postfix banded area series #391

Merged
merged 6 commits into from
Oct 2, 2019

Conversation

nickofthyme
Copy link
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

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
Member

@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
Member

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
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
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
Member

@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

src/chart_types/xy_chart/store/utils.ts Outdated Show resolved Hide resolved
@@ -40,7 +40,7 @@ export function formatTooltip(
displayName = name || `${id}`;
}

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

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
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
Member

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