Hooks #85
Hooks #85
Conversation
…instead of defaultProps
This fixes the issue of being able to typecheck the polymorphic props of the builtin placeholders and seems like a reasonable default to set.
There was a problem hiding this comment.
Hi @BrianMitchL! Looking good so far, I have left a few preliminary comments. Plus, I'm a bit confused by some of the typings for the top-level component Props - I'll take a second pass soon
| type?: undefined; | ||
| rows?: undefined; | ||
| color?: undefined; | ||
| showLoadingAnimation?: undefined; |
There was a problem hiding this comment.
I'm not sure I get the ?: undefined type for all of these props...
There was a problem hiding this comment.
Unless we use type guards for these props, this seemed to be the best way to set the values as undefined/not set. Otherwise, we can't even check if they're on the props object, as the compiler will say they don't exist. This pattern is used in the current version of the library. Is there a better way to do this?
There was a problem hiding this comment.
I see, I actually missed they were already defined this way in the previous version
Co-Authored-By: Giovanni Gonzaga <giovanni@buildo.io>
There was a problem hiding this comment.
@BrianMitchL LGTM 👏
Are there any other changes you'd like to consider before releasing a breaking version?
|
Nothing I can think of related to hooks! |
|
|
|
released as |
Closes #2520704
From the discussion in #83, I refactored the codebase to be built with functional components and hooks. Here's a change log in no particular order:
^16.8.0childrenandcustomPlaceholdertypes to beReactElementinstead ofReactNode(this is because therenderfunction in a class Component returns aReactNode, where functional components return aReactElement)ReactPlaceholdercomponent, use a default of3 for therows` props.ReactPlaceholderprops