-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
[SIP-5] Repair and refactor Horizon Chart #5690
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5690 +/- ##
==========================================
+ Coverage 63.43% 63.51% +0.08%
==========================================
Files 361 362 +1
Lines 22977 22947 -30
Branches 2558 2550 -8
==========================================
Hits 14575 14575
+ Misses 8387 8357 -30
Partials 15 15
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is much better! Thanks for refactoring 👍 Had one comment but otherwise lgtm
choices: [ | ||
['series', 'series'], | ||
['overall', 'overall'], | ||
['change', 'change'], | ||
], | ||
default: 'series', | ||
description: t('Defines how the color are attributed.'), | ||
description: t('series: Treat each series independently; overall: All series use the same scale; change: Show changes compared to the first data point in each series'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
className: '', | ||
width: 800, | ||
height: 20, | ||
bands: DEFAULT_COLORS.length >> 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any way to write this without bitwise operators for readability? like Math.floor(DEFAULT_COLORS.length / 2)
or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed. 👍
Ready to merge |
* Migrate horizon chart to react * remove unused code * rename files * update props * enable renderTrigger * fix canvas transform issue * Address Chris' comment (cherry picked from commit d7f06cb)
* Migrate horizon chart to react * remove unused code * rename files * update props * enable renderTrigger * fix canvas transform issue * Address Chris' comment (cherry picked from commit d7f06cb)
* Migrate horizon chart to react * remove unused code * rename files * update props * enable renderTrigger * fix canvas transform issue * Address Chris' comment
* Migrate horizon chart to react * remove unused code * rename files * update props * enable renderTrigger * fix canvas transform issue * Address Chris' comment
renderTrigger
on compatible propsBefore
After
@williaster @conglei