-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Statistic: enhance shorthand props and propTypes #390
Conversation
Current coverage is 94.86% (diff: 100%)@@ master #390 diff @@
==========================================
Files 85 85
Lines 1111 1110 -1
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
- Hits 1054 1053 -1
Misses 57 57
Partials 0 0
|
{ label: 'Views', value: '31,200' }, | ||
{ label: 'Members', value: 22 }, | ||
{ label: 'Members', value: '22' }, |
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.
FYI I think these would work as numbers as well.
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.
Nope 😄 22
is number, while '22'
is string.
Ideas?
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.
I think we should just change the propType validation to string/number. Since technically it could support node
as it ends up here:
<div {...rest} className={classes}>{children || value}</div>
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.
It cannot be node
, as it's used for key on StepGroup
.
Proptypes.oneOf([Proptypes.number, Proptypes.string])
I think that it's not good solution. This raises the question of the behavior of other properties.
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. OK, perhaps we only stick with string/number then.
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.
If we use string/number, we need allow it to all shorthand props.
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.
Hm, I suppose you are right about this. Feel free to update here or we can merge this and make that update next.
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.
I'll make a separate PR for this actually. The PR will resolve #391.
Thanks much for this! A few comments and we can merge. |
Replied on shorthand string/number types. |
I'll update it now with |
Great, I'll merge this tomorrow. It's getting late here! 🌔 |
01ebba6
to
b43d353
Compare
Released in |
1. Update README.md
I forget mark
<Step>
as implemented.2. Update usage of
Message
<Message>
was updated to v1 API, I've updated its usage in docs.3. Update Statistic's docs
I've updated naming of docs to the most recent pattern.
4. Update prop handling
StatisticLabel
andStatisticLabel
.StatisticGroup
:key
handling for children (antipattern withindex
usage);prop
validation ofitems
withProptypes.shape
.