-
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] Refactor rose #5763
[SIP-5] Refactor rose #5763
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5763 +/- ##
==========================================
- Coverage 63.8% 63.78% -0.02%
==========================================
Files 364 364
Lines 23066 23073 +7
Branches 2568 2568
==========================================
Hits 14717 14717
- Misses 8334 8341 +7
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.
Much more readable! 💯 One suggestion / question for the data proptype otherwise lgtm!
import './rose.css'; | ||
|
||
const propTypes = { | ||
// Data is an object hashed by numeric value, perhaps timestamp |
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.
Does it work do specify this like this?
data: PropTypes.objectOf(
PropTypes.arrayOf(PropTypes.shape({ ... }),
),
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.
Good call. Will update
* refactor rose * update proptypes (cherry picked from commit 86f99b0)
* refactor rose * update proptypes
* refactor rose * update proptypes
slice
andformData
from chart.Tested locally
@williaster @conglei