Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

fix(content): fixing the type of the content prop in all components #528

Merged
merged 18 commits into from
Dec 3, 2018

Conversation

mnajdova
Copy link
Contributor

@mnajdova mnajdova commented Nov 27, 2018

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

export interface ContentShorthandComponentProps {
  /** Shorthand for primary content. */
  content?: ShorthandValue
}

export const contentShorthandComponentPropsTypes = {
  content: customPropTypes.itemShorthand,
}

2. The content is used as a ReactNode

export interface ContentNodeComponentProps {
  /** Shorthand for primary content. */
  content?: React.ReactNode
}

export const contentNodeComponentPropsTypes = {
  content: customPropTypes.contentShorthand,
}

In addition, the customPropTypes.itemShorthand was changed in the following manner:

Before

export const itemShorthand = every([
  disallow(['children']),
  PropTypes.oneOfType([
    PropTypes.node,
    PropTypes.object,
    PropTypes.arrayOf(PropTypes.oneOfType([PropTypes.node, PropTypes.object])),
  ]),
])

After

export const itemShorthand = every([
  disallow(['children']),
  PropTypes.oneOfType([
    PropTypes.node,
    PropTypes.object,
  ]),
])

With this change, it corresponds to the TS definition ShorthandValue:

export type ShorthandValue = React.ReactNode | Props

Fixes #504

-updated component to use the right type of the content
@codecov
Copy link

codecov bot commented Nov 27, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@11e36d5). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #528   +/-   ##
=========================================
  Coverage          ?   87.87%           
=========================================
  Files             ?       42           
  Lines             ?     1419           
  Branches          ?      212           
=========================================
  Hits              ?     1247           
  Misses            ?      167           
  Partials          ?        5
Impacted Files Coverage Δ
src/index.ts 100% <ø> (ø)
src/components/Text/Text.tsx 100% <ø> (ø)
src/components/Header/HeaderDescription.tsx 100% <ø> (ø)
src/components/Popup/PopupContent.tsx 100% <ø> (ø)
src/components/Divider/Divider.tsx 100% <ø> (ø)
src/components/Accordion/AccordionTitle.tsx 63.15% <ø> (ø)
src/components/Input/Input.tsx 100% <ø> (ø)
src/components/Slot/Slot.tsx 100% <ø> (ø)
src/components/Accordion/AccordionContent.tsx 78.57% <ø> (ø)
src/components/Chat/ChatMessage.tsx 96.15% <ø> (ø)
... and 29 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 11e36d5...de2468b. Read the comment docs.

@mnajdova mnajdova changed the title [WIP] fix(content): fixing the type of the content prop in all components fix(content): fixing the type of the content prop in all components Nov 27, 2018
manajdov added 2 commits November 27, 2018 15:56
…usages

-changed Portal's props' interface content type to correspond to the propTypes
# Conflicts:
#	src/components/Animation/Animation.tsx
#	src/lib/commonPropInterfaces.ts
Copy link
Member

@layershifter layershifter left a 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(),

@layershifter
Copy link
Member

It will be awesome to have a similar generic for UIComponentProps, but I don't have a good idea there.

@layershifter layershifter added needs author feedback Author's opinion is asked and removed 🚀 ready for review labels Nov 29, 2018
@mnajdova
Copy link
Contributor Author

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
Copy link
Contributor

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
}

* Used to set content when using childrenApi - internal only
* @docSiteIgnore
*/
children?: React.ReactChild
Copy link
Contributor

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

/** 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 {
Copy link
Contributor

@kuzhelov kuzhelov Nov 29, 2018

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=...> { ... }

content: customPropTypes.itemShorthand,
}

export const contentNodeComponentPropsTypes = {
content: customPropTypes.contentShorthand,
Copy link
Contributor

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]),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

className: PropTypes.string,
}),
...(content && {
content: content === true ? customPropTypes.contentShorthand : customPropTypes.itemShorthand,
Copy link
Member

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,

Copy link
Member

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

Copy link
Contributor Author

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(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

Copy link
Member

@layershifter layershifter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great cleanup 👍

@kuzhelov kuzhelov added ready for merge and removed needs author feedback Author's opinion is asked labels Dec 3, 2018
@mnajdova mnajdova merged commit d88e338 into master Dec 3, 2018
@layershifter layershifter deleted the fix/content-type branch December 3, 2018 13:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants