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(flamegraph): solve animation regression #1541

Merged
merged 2 commits into from
Dec 30, 2021

Conversation

monfera
Copy link
Contributor

@monfera monfera commented Dec 29, 2021

Summary

Restored tweening (animation) to flamegraph that stopped working at 6db2677

Details

The work that moved numerous config options into the theme turned the previously mandatory animation property optional. This watered up preexisting TS type checks, as optionals often do, and we don't yet have mid-flight VRT for (currently alpha) animations, so it flew under the radar.

The fix restores the mandatory nature of animation and supplies a default zero duration for it.
In the future it might become part of the theme, would need a convo.

Issues

Closes #1540

Checklist

  • The proper chart type label has been added (e.g. :xy, :partition)
  • The proper feature labels have been added (e.g. :interactions, :axis)
  • The :theme label has been added and the @elastic/eui-design team has been pinged when there are Theme API changes
  • All related issues have been linked (i.e. closes #123, fixes #123)
  • New public API exports have been added to packages/charts/src/index.ts
  • Unit tests have been added or updated to match the most common scenarios
  • The proper documentation and/or storybook story has been added or updated
  • The code has been checked for cross-browser compatibility (Chrome, Firefox, Safari, Edge)
  • Visual changes have been tested with all available themes including dark, light, eui-dark & eui-light

@monfera monfera added :interactions Interactions related issue :partition Partition/PieChart/Donut/Sunburst/Treemap chart related :flamegraph Flame/Icicle chart related issues labels Dec 29, 2021
@monfera monfera marked this pull request as draft December 29, 2021 15:55
@monfera monfera marked this pull request as ready for review December 29, 2021 16:19
@@ -511,6 +512,7 @@ export function shapeViewModel(
pickQuads,
outerRadius,
chartDimensions,
animation,
Copy link
Contributor Author

@monfera monfera Dec 29, 2021

Choose a reason for hiding this comment

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

This line is the actual fix, and the other change (restoring mandatory nature of the property) just ensures it can't get lost accidentally

@monfera monfera requested a review from rshen91 December 29, 2021 17:04
Copy link
Contributor

@rshen91 rshen91 left a comment

Choose a reason for hiding this comment

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

Changes LGTM - tested locally on Chrome

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

@monfera monfera merged commit 5ec6037 into elastic:master Dec 30, 2021
nickofthyme pushed a commit that referenced this pull request Jan 5, 2022
# [42.0.0](v41.0.1...v42.0.0) (2022-01-05)

### Bug Fixes

* **flamegraph:** solve animation regression occurring with 6db2677 ([#1541](#1541)) ([5ec6037](5ec6037)), closes [#1540](#1540)
* **heatmap:** render empty state ([#1532](#1532)) ([59002df](59002df))
* **waffle:** fix strange 0 text in legend item extra when label is 0 ([#1538](#1538)) ([72224b9](72224b9))

### Features

* **goal:** add valueFormatter for tooltip ([#1529](#1529)) ([8139973](8139973))
* **heatmap:** add axis titles ([#1503](#1503)) ([a87325d](a87325d))
* **types:** improve generic types in specs, and spec prop types ([#1421](#1421)) ([562929e](562929e))

### BREAKING CHANGES

* **types:** The `xAccessor` and `yAccessor` are now required on all xy chart specs. Stronger typing on `data` prop that may cause type errors when using untyped array (i.e. `const arr: never[] = []`). Other minor type changes related to spec types.
* **heatmap:** The heatmap yAxisLabel.padding style type is changed from Pixel | Partial<Padding> to Pixels | Padding. The heatmap axis labels are now correctly subjected to padding calculations and it will result in a slightly different position of labels.

Co-authored-by: Marco Vettorello <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:flamegraph Flame/Icicle chart related issues :interactions Interactions related issue :partition Partition/PieChart/Donut/Sunburst/Treemap chart related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Flamegraph] tweening stopped (regression)
3 participants