Conversation
nicko-winner
commented
Apr 10, 2020
nicko-winner
commented
|
Deploy preview for helix-react ready! Built with commit 504885e |
michaelmang
left a comment
michaelmang
left a comment
There was a problem hiding this comment.
👍 Looks good. I think it would be good at some point to do a pull request to add eslint and prettier to have consistent rules and code formatting.
| status: PropTypes.string, | ||
| cta: PropTypes.string, | ||
| onOpen: PropTypes.func, | ||
| onClose: PropTypes.func |
There was a problem hiding this comment.
I think it makes sense to specify which props should be marked with isRequired.
There was a problem hiding this comment.
I think just children may be the only required item do you see others?
There was a problem hiding this comment.
I think you are correct. Also, I noticed that the persist prop is missing from the propTypes.
Do we want to default the persist and type props?
There was a problem hiding this comment.
good catch. i'm tempted to not use default propTypes and just let helix-ui pick the defaults. That way our wrapper defaults wont get in the way of what Helix may set as defaults in the future. (makes the wrapper slightly more light weight). @100stacks you have an any opinion on if we should be setting defaults props/attributes before passing them on to helix?
There was a problem hiding this comment.
@HelixDesignSystem/helix-react-core I think we left this open during our sync today. Balancing explicit defaults vs. implied HelixUI defaults.
We can address again as needed.
|
Fixes #19 ( |
| status: PropTypes.string, | ||
| cta: PropTypes.string, | ||
| onOpen: PropTypes.func, | ||
| onClose: PropTypes.func |
There was a problem hiding this comment.
@HelixDesignSystem/helix-react-core I think we left this open during our sync today. Balancing explicit defaults vs. implied HelixUI defaults.
We can address again as needed.
3 participants
