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

fix(last_value): compute last value for non stacked series #261

Merged
merged 2 commits into from
Jul 10, 2019

Conversation

markov00
Copy link
Member

@markov00 markov00 commented Jul 5, 2019

Summary

Fix an error introduced in #250 where the last value on the legend was undefined for non stacked series.

When using the stack percentage mode, the data values are computed on a percentage scale. As we want to display the percent value and not the original value when displaying a tooltip, I've changed the place where the lastValue value is captured: from the original dataset to the formatted one.

The commit fix the the fact that the last value was computed only for stacked series.

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

The percentage stacked mode implies to use the formatted value to be shown in the tooltips and on
the legend. I moved the computation from the splittes series to it's own function after the series
set is formatted. The commit fix the current status where the last value was computed only for
stacked series.
@markov00 markov00 requested a review from emmacunningham July 5, 2019 12:53
@markov00 markov00 added bug Something isn't working :legend Legend related issue labels Jul 5, 2019
@codecov-io
Copy link

codecov-io commented Jul 5, 2019

Codecov Report

Merging #261 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #261      +/-   ##
==========================================
+ Coverage   97.84%   97.85%   +<.01%     
==========================================
  Files          36       36              
  Lines        2693     2698       +5     
  Branches      607      609       +2     
==========================================
+ Hits         2635     2640       +5     
  Misses         51       51              
  Partials        7        7
Impacted Files Coverage Δ
src/lib/series/series.ts 100% <ø> (ø) ⬆️
src/state/utils.ts 96.58% <100%> (+0.13%) ⬆️

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 e727605...a79104a. Read the comment docs.

Copy link
Contributor

@emmacunningham emmacunningham left a comment

Choose a reason for hiding this comment

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

Looks like something weird is going on when the series is clustered but also stackAsPercentage={true}:

display_value

@markov00
Copy link
Member Author

markov00 commented Jul 8, 2019

Looks like something weird is going on when the series is clustered but also stackAsPercentage={true}:

Are you referring to the legend showing 600 % and 200 % ? I will update the storybook for that, the wrong thing here is that I'm multipling by 100 the value on the tickFormatter either for the stacked as percentage mode and for non stacked one

@emmacunningham
Copy link
Contributor

Looks like something weird is going on when the series is clustered but also stackAsPercentage={true}:

Are you referring to the legend showing 600 % and 200 % ? I will update the storybook for that, the wrong thing here is that I'm multipling by 100 the value on the tickFormatter either for the stacked as percentage mode and for non stacked one

Yes, the legend display value is what I was referring to. I wasn't exactly sure how we want to handle this since clustered + stackAsPercentage isn't an ideal configuration, but I see now how that's just a Storybook configuration thing that can be easily solved :D

Copy link
Contributor

@emmacunningham emmacunningham left a comment

Choose a reason for hiding this comment

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

besides the storybook issue, things working locally & code LGTM

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.

Tested locally, LGTM.

@markov00 markov00 merged commit 803c34e into elastic:master Jul 10, 2019
@markov00 markov00 deleted the fix_last_value branch July 10, 2019 10:52
markov00 pushed a commit that referenced this pull request Jul 10, 2019
## [7.2.1](v7.2.0...v7.2.1) (2019-07-10)

### Bug Fixes

* **last_value:** compute last value for non stacked series ([#261](#261)) ([803c34e](803c34e))
@markov00
Copy link
Member Author

🎉 This PR is included in version 7.2.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Jul 10, 2019
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
## [7.2.1](elastic/elastic-charts@v7.2.0...v7.2.1) (2019-07-10)

### Bug Fixes

* **last_value:** compute last value for non stacked series ([opensearch-project#261](elastic/elastic-charts#261)) ([5bdaecd](elastic/elastic-charts@5bdaecd))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working :legend Legend related issue released Issue released publicly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants