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

mnajdova · 2018-11-27T11:16:51Z

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 · 2018-11-27T11:25:20Z

Codecov Report

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

@@            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.

-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

layershifter

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 · 2018-11-29T12:43:02Z

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

mnajdova · 2018-11-29T13:30:33Z

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.

kuzhelov · 2018-11-29T15:19:07Z

src/components/Animation/Animation.tsx

+   *  Content for childrenApi
+   *  @docSiteIgnore
+   */
+  children?: React.ReactChild


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 }

kuzhelov · 2018-11-29T15:23:29Z

src/components/Ref/Ref.tsx

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


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

kuzhelov · 2018-11-29T15:26:55Z

src/lib/commonPropInterfaces.ts

  /** 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 {


would suggest to still use single type (ContentComponentProps ) with generic parameter:

ContentComponentProps<TContent=...> { ... }

kuzhelov · 2018-11-29T15:32:20Z

src/lib/commonPropTypes.ts

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


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

kuzhelov · 2018-11-29T15:32:51Z

src/lib/customPropTypes.tsx

-    PropTypes.object,
-    PropTypes.arrayOf(PropTypes.oneOfType([PropTypes.node, PropTypes.object])),
-  ]),
+  PropTypes.oneOfType([PropTypes.node, PropTypes.object]),


…se this

…ed the components

layershifter · 2018-11-30T16:55:53Z

src/lib/commonPropTypes.ts

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


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,

And use as defaults:

children: 'node' => PropTypes.node

content: 'content' => customPropTypes.contentShorthand

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.

layershifter · 2018-12-03T09:18:49Z

src/components/Accordion/AccordionTitle.tsx

-    ...commonUIComponentPropTypes,
-    ...childrenComponentPropTypes,
-    ...contentComponentPropsTypes,
+    ...commonPropTypes.createCommon(),


layershifter

Great cleanup 👍

… into fix/content-type

src/components/Grid/Grid.tsx

-refactored Content related prop interfaces

f34e025

-updated component to use the right type of the content

mnajdova requested review from dvdzkwsk, kuzhelov, levithomason and miroslavstastny as code owners November 27, 2018 11:16

-fixed itemShorthand custom Prop type definition

57b175f

kuzhelov added the 🚧 WIP label Nov 27, 2018

manajdov added 2 commits November 27, 2018 14:22

-refactored props' interfaces and propTypes imports

d2ff44c

-fixed children types in the Ref and Transition for requiring only child

-fixed children types in the Ref and Transition for requiring only child

c0d4a05

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

mnajdova added 🚀 ready for review and removed 🚧 WIP labels Nov 27, 2018

manajdov added 2 commits November 27, 2018 15:56

-added defaults for the generics in UIComponentProps and updated the …

c9e7c7a

…usages -changed Portal's props' interface content type to correspond to the propTypes

Merge branch 'master' into fix/content-type

8ce20c2

# Conflicts: # src/components/Animation/Animation.tsx # src/lib/commonPropInterfaces.ts

layershifter reviewed Nov 29, 2018

View reviewed changes

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

kuzhelov reviewed Nov 29, 2018

View reviewed changes

manajdov added 3 commits November 29, 2018 16:39

-added commonPropTypes factory and updated components' propTypes to u…

635d61d

…se this

-fixed compiling errors

1a36cc3

-added generic to the Content and Children props interfaces and updat…

89a21bd

…ed the components

layershifter reviewed Nov 30, 2018

View reviewed changes

manajdov added 2 commits November 30, 2018 18:32

-addressed comments

5184e46

-improved common props factory

f5392fd

layershifter reviewed Dec 3, 2018

View reviewed changes

Merge branch 'master' into fix/content-type

b597b70

layershifter approved these changes Dec 3, 2018

View reviewed changes

-fixed Tree component typings

786c14c

kuzhelov approved these changes Dec 3, 2018

View reviewed changes

kuzhelov added ready for merge and removed needs author feedback Author's opinion is asked labels Dec 3, 2018

layershifter and others added 4 commits December 3, 2018 12:23

Merge branch 'master' into fix/content-type

27eb82d

-final fixes before merge

b4ac819

Merge branch 'fix/content-type' of https://github.com/stardust-ui/react…

56de714

… into fix/content-type

-updated changelog

39bf3ba

kuzhelov reviewed Dec 3, 2018

View reviewed changes

src/components/Grid/Grid.tsx Show resolved Hide resolved

-reverted changes for the Inout component because of regressions

de2468b

mnajdova merged commit d88e338 into master Dec 3, 2018

layershifter deleted the fix/content-type branch December 3, 2018 13:04

bmdalex mentioned this pull request Dec 13, 2018

Replace contentShorthand with itemShorthand #504

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mnajdova commented Nov 27, 2018 •

edited by bmdalex

Loading

codecov bot commented Nov 27, 2018 •

edited

Loading

layershifter left a comment •

edited

Loading

layershifter commented Nov 29, 2018

mnajdova commented Nov 29, 2018

kuzhelov Nov 29, 2018

kuzhelov Nov 29, 2018

kuzhelov Nov 29, 2018 •

edited

Loading

kuzhelov Nov 29, 2018

kuzhelov Nov 29, 2018

layershifter Nov 30, 2018

layershifter Nov 30, 2018

mnajdova Nov 30, 2018

layershifter Dec 3, 2018

layershifter left a comment

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

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

Conversation

mnajdova commented Nov 27, 2018 • edited by bmdalex Loading

1. The content is used as a shorthand

2. The content is used as a ReactNode

Before

After

codecov bot commented Nov 27, 2018 • edited Loading

Codecov Report

layershifter left a comment • edited Loading

Choose a reason for hiding this comment

Improve propTypes

layershifter commented Nov 29, 2018

mnajdova commented Nov 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuzhelov Nov 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

layershifter left a comment

Choose a reason for hiding this comment

mnajdova commented Nov 27, 2018 •

edited by bmdalex

Loading

codecov bot commented Nov 27, 2018 •

edited

Loading

layershifter left a comment •

edited

Loading

kuzhelov Nov 29, 2018 •

edited

Loading