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.

Sign up for GitHub

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

Jump to bottom

fix: build issues and tooltip formatting issues #810

Merged
merged 3 commits into from
Sep 8, 2020
Merged

fix: build issues and tooltip formatting issues #810

merged 3 commits into from
Sep 8, 2020

Conversation

nickofthyme
Copy link
Collaborator

Summary

Fix x-axis and tooltip header formatting caused by #802, by adding formattedValue string to TooltipValue in addition to actual value.

The issue in #802 changed value from the given value to the formatted string value when using the tooltip.headerFormatter prop.

Also fixes...

  • add --no-version-updates flag to storybook start script
  • upgrade @storybook/ dependencies
  • remove @types/react causing type error in dist library check
  • move redux optionalDependencies to devDependencies

Checklist

  • Any consumer-facing exports were added to src/index.ts (and stories only import from ../src except for test data & storybook)
  • 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

@nickofthyme
fix: build issues and tooltip formatting issues
690a33f 
- add `formattedValue` to `TooltipValue`
- fix x-axis and tooltip header formatting
- add `--no-version-updates` flag to storybook start script
- upgrade `@storybook/` dependencies
- remove `@types/react` causing type error in dist library check
- move redux `optionalDependencies` to `devDependencies`
@nickofthyme nickofthyme added bug Something isn't working :tooltip Related to hover tooltip dependencies Pull requests that update a dependency file :build Build tools / dependencies labels Sep 8, 2020
@codecov-commenter
Copy link

codecov-commenter commented Sep 8, 2020 •
edited
Loading

Codecov Report

Merging #810 into master will decrease coverage by 0.07%.
The diff coverage is 12.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #810      +/-   ##
==========================================
- Coverage   74.35%   74.27%   -0.08%     
==========================================
  Files         273      273              
  Lines        9322     9330       +8     
  Branches     2011     2010       -1     
==========================================
- Hits         6931     6930       -1     
- Misses       2383     2392       +9     
  Partials        8        8
Flag Coverage Δ
#unittests 74.27% <12.50%> (-0.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.../chart_types/goal_chart/state/selectors/tooltip.ts 38.46% <ø> (ø)
...t_types/partition_chart/state/selectors/tooltip.ts 29.16% <0.00%> (-1.27%) ⬇️
src/components/chart.tsx 69.04% <0.00%> (-5.32%) ⬇️
src/components/tooltip/tooltip.tsx 69.23% <0.00%> (-1.56%) ⬇️
src/specs/settings.tsx 87.09% <ø> (ø)
src/chart_types/xy_chart/tooltip/tooltip.ts 74.35% <50.00%> (ø)
src/chart_types/xy_chart/legend/legend.ts 80.95% <100.00%> (-0.45%) ⬇️

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 0e6353f...aecfcaa. Read the comment docs.

@nickofthyme
Copy link
Collaborator Author

jenkins retest this please

monfera
monfera approved these changes Sep 8, 2020
View reviewed changes
Copy link
Contributor

@monfera monfera left a comment

Choose a reason for hiding this comment

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

LGTM on an less than full length look. Would be good to have a visual regression test that passes with this change but wouldn't have passed before this, but it can be added subsequently to merging this PR

@monfera
Copy link
Contributor

monfera commented Sep 8, 2020

jenkins retest this please

@nickofthyme
Copy link
Collaborator Author

Would be good to have a visual regression test that passes with this change but wouldn't have passed before this, but it can be added subsequently to merging this PR

Added in bebb62c

@nickofthyme nickofthyme merged commit 74d9ae0 into elastic:master Sep 8, 2020
@nickofthyme nickofthyme deleted the fix/build-and-formatting branch September 8, 2020 21:52
markov00 pushed a commit that referenced this pull request Sep 8, 2020
@semantic-release-bot
chore(release): 21.1.1 [skip ci]
c9a83d7 
## [21.1.1](v21.1.0...v21.1.1) (2020-09-08)

### Bug Fixes

* build issues and tooltip formatting issues ([#810](#810)) ([74d9ae0](74d9ae0))
@markov00
Copy link
Member

markov00 commented Sep 8, 2020

🎉 This PR is included in version 21.1.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Sep 8, 2020
@timroes timroes mentioned this pull request Sep 22, 2020
Closed
nickofthyme added a commit that referenced this pull request Sep 22, 2020
@nickofthyme
fix: breaking change in patch release of 21.1.1
d0ddc45 
breaking change caused by changes in #810 see #830 for more info

BREAKING CHANGE: `TooltipValue.value` is now raw value and `TooltipValue.formattedValue` is now the
string formatted value.

fix #810
markov00 pushed a commit that referenced this pull request Sep 22, 2020
@semantic-release-bot
chore(release): 22.0.0 [skip ci]
25251b3 
# [22.0.0](v21.3.2...v22.0.0) (2020-09-22)

### Bug Fixes

* breaking change in patch release of 21.1.1 ([d0ddc45](d0ddc45)), closes [#810](#810)

### BREAKING CHANGES

* caused by changes in #810 see #830 for more info
* `TooltipValue.value` is now raw value and `TooltipValue.formattedValue` is now the
string formatted value.
@markov00
Copy link
Member

🎉 This issue has been resolved in version 22.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

darnautov pushed a commit to darnautov/elastic-charts that referenced this pull request Sep 22, 2020
@nickofthyme @darnautov
fix: breaking change in patch release of 21.1.1
5c87e27 
breaking change caused by changes in elastic#810 see elastic#830 for more info

BREAKING CHANGE: `TooltipValue.value` is now raw value and `TooltipValue.formattedValue` is now the
string formatted value.

fix elastic#810
@nickofthyme nickofthyme mentioned this pull request Oct 1, 2020
Merged
1 task
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
@semantic-release-bot
chore(release): 21.1.1 [skip ci]
aab9902 
## [21.1.1](elastic/elastic-charts@v21.1.0...v21.1.1) (2020-09-08)

### Bug Fixes

* build issues and tooltip formatting issues ([opensearch-project#810](elastic/elastic-charts#810)) ([738283a](elastic/elastic-charts@738283a))
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
@semantic-release-bot
chore(release): 22.0.0 [skip ci]
fd8cc2a 
# [22.0.0](elastic/elastic-charts@v21.3.2...v22.0.0) (2020-09-22)

### Bug Fixes

* breaking change in patch release of 21.1.1 ([9ae64a9](elastic/elastic-charts@9ae64a9)), closes [opensearch-project#810](elastic/elastic-charts#810)

### BREAKING CHANGES

* caused by changes in opensearch-project#810 see opensearch-project#830 for more info
* `TooltipValue.value` is now raw value and `TooltipValue.formattedValue` is now the
string formatted value.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@monfera monfera monfera approved these changes

@markov00 markov00 Awaiting requested review from markov00

Assignees
No one assigned
Labels
bug Something isn't working :build Build tools / dependencies dependencies Pull requests that update a dependency file released Issue released publicly :tooltip Related to hover tooltip
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nickofthyme @codecov-commenter @monfera @markov00