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(animation): re-enabled animateData prop to disable animation #129

Merged
merged 2 commits into from
Mar 29, 2019
Merged

fix(animation): re-enabled animateData prop to disable animation #129

merged 2 commits into from
Mar 29, 2019

Conversation

markov00
Copy link
Member

Summary

The animateData prop was not used. This PR reenable this functionality and add a max thresholds of bars and lines/area points before disabling automatically the animation.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

  • 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

@markov00 markov00 added bug Something isn't working :chart Chart element related issue :specs Chart specifications related issue labels Mar 27, 2019
@markov00 markov00 requested a review from emmacunningham March 27, 2019 18:00
@markov00 markov00 mentioned this pull request Mar 27, 2019
Closed
2 tasks
@codecov-io
Copy link

Codecov Report

Merging #129 into master will decrease coverage by 0.58%.
The diff coverage is 60.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #129      +/-   ##
==========================================
- Coverage   91.07%   90.48%   -0.59%     
==========================================
  Files          31       31              
  Lines        1456     1482      +26     
  Branches      163      164       +1     
==========================================
+ Hits         1326     1341      +15     
- Misses        116      127      +11     
  Partials       14       14
Impacted Files Coverage Δ
src/state/chart_state.ts 95.52% <100%> (ø) ⬆️
src/state/utils.ts 70.76% <57.69%> (-2.35%) ⬇️

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 cc2cc5e...1fbd563. Read the comment docs.

emmacunningham
emmacunningham approved these changes Mar 28, 2019
View reviewed changes
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.

Code LGTM, just a few non-blocking comments to consider

src/state/utils.ts Outdated Show resolved Hide resolved
src/state/utils.ts Outdated Show resolved Hide resolved
src/state/chart_state.ts

this.canDataBeAnimated = isChartAnimatable(seriesGeometries.geometriesCounts, this.animateData);
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we can rename this.animateData to something that indicates that it is a boolean setting? this.isAnimationEnabled

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes absolutely, I prefer to address this into #121
so we can clean up a bit the interface with one single breaking change instead of a lot of small ones

@markov00 markov00 mentioned this pull request Mar 29, 2019
Closed
8 tasks
@markov00 markov00 merged commit 32b4263 into elastic:master Mar 29, 2019
markov00 pushed a commit that referenced this pull request Mar 29, 2019
@semantic-release-bot
chore(release): 3.4.5 [skip ci]
092e7f4 
## [3.4.5](v3.4.4...v3.4.5) (2019-03-29)

### Bug Fixes

* **animation:** re-enabled animateData prop to disable animation ([#129](#129)) ([32b4263](32b4263))
* **specs:** limit xScaleType to linear, time and ordinal ([#127](#127)) ([59c3b70](59c3b70)), closes [#122](#122)
@markov00
Copy link
Member Author

🎉 This PR is included in version 3.4.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Mar 29, 2019
@markov00 markov00 deleted the reenable-animation branch April 16, 2019 12:06
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
@semantic-release-bot
chore(release): 3.4.5 [skip ci]
b66cce8 
## [3.4.5](elastic/elastic-charts@v3.4.4...v3.4.5) (2019-03-29)

### Bug Fixes

* **animation:** re-enabled animateData prop to disable animation ([opensearch-project#129](elastic/elastic-charts#129)) ([c15339c](elastic/elastic-charts@c15339c))
* **specs:** limit xScaleType to linear, time and ordinal ([opensearch-project#127](elastic/elastic-charts#127)) ([30f2f7e](elastic/elastic-charts@30f2f7e)), closes [opensearch-project#122](elastic/elastic-charts#122)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emmacunningham emmacunningham emmacunningham approved these changes

Assignees
No one assigned
Labels
bug Something isn't working :chart Chart element related issue released Issue released publicly :specs Chart specifications related issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@markov00 @codecov-io @emmacunningham