-
Notifications
You must be signed in to change notification settings - Fork 55
fix(content): fixing the type of the content prop in all components #528
Conversation
-updated component to use the right type of the content
Codecov Report
@@ Coverage Diff @@
## master #528 +/- ##
=========================================
Coverage ? 87.87%
=========================================
Files ? 42
Lines ? 1419
Branches ? 212
=========================================
Hits ? 1247
Misses ? 167
Partials ? 5
Continue to review full report at Codecov.
|
-fixed children types in the Ref and Transition for requiring only child
…usages -changed Portal's props' interface content type to correspond to the propTypes
# Conflicts: # src/components/Animation/Animation.tsx # src/lib/commonPropInterfaces.ts
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.
Improve propTypes
We don't use propTypes
to generate componentInfo
, so may be we can just create a factory for them?
- ...commonPropTypes.commonUIComponentPropTypes,
- ...commonPropTypes.childrenComponentPropTypes,
+ ...customPropTypes.createCommon({
+ content: false,
+ }}
- ...commonPropTypes.commonUIComponentPropTypes,
- ...commonPropTypes.childrenComponentPropTypes,
- ...commonPropTypes.contentShorthandComponentPropsTypes,
+ ...customPropTypes.createCommon({
+ content: 'shorthand',
+ }}
- propTypes: {
- ...commonPropTypes.commonUIComponentPropTypes,
- ...commonPropTypes.contentNodeComponentPropsTypes,
- ...commonPropTypes.childrenComponentPropTypes,
- },
+ propTypes: customPropTypes.createCommon(),
It will be awesome to have a similar generic for |
Thanks @layershifter this is actually a great idea about the propTypes. Will add this functionality and will try to think about something similar for the props' interfaces. |
* Content for childrenApi | ||
* @docSiteIgnore | ||
*/ | ||
children?: React.ReactChild |
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.
as description is duplicated now, I would rather suggest to still encapsulate it at the single place (ChildrenComponentProps
), and just introduce optional generic parameter to this type:
ChildrenComponentProps<TChildren=React.ReactChildren> {
/**
* Content for childrenApi
* @docSiteIgnore
*/
children?: TChildren
}
src/components/Ref/Ref.tsx
Outdated
* Used to set content when using childrenApi - internal only | ||
* @docSiteIgnore | ||
*/ | ||
children?: React.ReactChild |
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 would not this see to be internal only, as in case if Ref will be exposed to the client from Stardust, either of the following is absolutely legal:
<Ref innerRef={...}>
<MyCustomComponent>
</Ref>
or its equivalent:
<Ref innerRef={...} children={<MyCustomComponent>} />
So, thus, would suggest to use already existing prop type here as well (ChildrenComponentProps
), as its description for children
will be an exact match for this component as well
src/lib/commonPropInterfaces.ts
Outdated
/** An element type to render as (string or function). */ | ||
as?: any | ||
|
||
/** Additional CSS class name(s) to apply. */ | ||
className?: string | ||
} | ||
|
||
export interface ContentComponentProps { | ||
export interface ContentShorthandComponentProps { |
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.
would suggest to still use single type (ContentComponentProps
) with generic parameter:
ContentComponentProps<TContent=...> { ... }
src/lib/commonPropTypes.ts
Outdated
content: customPropTypes.itemShorthand, | ||
} | ||
|
||
export const contentNodeComponentPropsTypes = { | ||
content: customPropTypes.contentShorthand, |
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.
would suggest to rename this custom prop type - while underlying type is correct, its name is misleading. Something like nodeContent
would be a better choice here: in either case, I would rather not expect shorthand
word being used here
PropTypes.object, | ||
PropTypes.arrayOf(PropTypes.oneOfType([PropTypes.node, PropTypes.object])), | ||
]), | ||
PropTypes.oneOfType([PropTypes.node, PropTypes.object]), |
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.
👍
src/lib/commonPropTypes.ts
Outdated
className: PropTypes.string, | ||
}), | ||
...(content && { | ||
content: content === true ? customPropTypes.contentShorthand : customPropTypes.itemShorthand, |
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 that children
and content
should accept other values:
-
children: false
=>{}
-
children: 'node'
=>PropTypes.node
-
children: 'element'
=>PropTypes.element
-
content: false
=>{}
-
content: 'content'
=>customPropTypes.contentShorthand
-
content: 'item'
=>customPropTypes.itemShorthand,
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.
And use as defaults:
children: 'node'
=>PropTypes.node
content: 'content'
=>customPropTypes.contentShorthand
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.
Changed, just note for the content, the values available are boolean | 'node' | 'shorthand'
, as it is aligned more with the renamed contentShorthand -> nodeContent
custom prop type. Please take a look.
...commonUIComponentPropTypes, | ||
...childrenComponentPropTypes, | ||
...contentComponentPropsTypes, | ||
...commonPropTypes.createCommon(), |
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.
❤️
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.
Great cleanup 👍
This PR introduces the correct typings in the props' interface as well as in the propTypes in all components regarding the content property. For this purpose two different interfaces and commonPropTypes definition are introduced:
1. The content is used as a shorthand
2. The content is used as a ReactNode
In addition, the customPropTypes.itemShorthand was changed in the following manner:
Before
After
With this change, it corresponds to the TS definition ShorthandValue:
Fixes #504