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

feat(utils): replace react-docgen with typescript #460

Merged
merged 15 commits into from
Nov 14, 2018

Conversation

mnajdova
Copy link
Contributor

@mnajdova mnajdova commented Nov 9, 2018

RFC

This PR aims to replace react-docgen with alternative that will generate the info files for the components using the typescript interfaces. The PR introduces the react-docgen-typescript library, that out of the interfaces for the props of the component, generates json in the same format as the react-docgen library. I added one interface for the UIComponentProps with some common props that can be extended in the other component's props.

Example

For example, here is how the Button component prop section looks now, in comparison with the previous:

NOW

image

PREV

image

You can see that the types of the props are much descriptive with the new approach, but there are still some things that needs to be taken into account.
There is some bug I guess, because the types of the @params used inside the JSDoc are not picked up. I created issue for this on their github repo. In addition, the default value of the props are taken just if the value is some primitive type. However for the second, there is an alternative way of defining, by adding @default in the comment of the property.

Please share your feedback, so that we can make progress with this on the other components as well. @alinais @Bugaa92 @levithomason @kuzhelov @miroslavstastny @layershifter

Updated components

  • Accordion
  • Attachment
  • Avatar
  • Button
  • Chat
  • Divider
  • Form
  • Grid
  • Header
  • Icon
  • Image
  • Input
  • ItemLayout
  • Label
  • Layout
  • List
  • Menu
  • Popup
  • Portal
  • Provider
  • Text
  • Radio Group
  • Segment
  • Status
  • Slot

@codecov
Copy link

codecov bot commented Nov 9, 2018

Codecov Report

Merging #460 into master will increase coverage by 0.25%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #460      +/-   ##
=========================================
+ Coverage   88.94%   89.2%   +0.25%     
=========================================
  Files          41      41              
  Lines        1384    1417      +33     
  Branches      202     202              
=========================================
+ Hits         1231    1264      +33     
  Misses        149     149              
  Partials        4       4
Impacted Files Coverage Δ
src/components/Provider/ProviderConsumer.tsx 100% <ø> (ø) ⬆️
src/components/Label/Label.tsx 87.5% <ø> (+0.26%) ⬆️
src/components/Chat/Chat.tsx 91.66% <100%> (+0.36%) ⬆️
src/components/Avatar/Avatar.tsx 100% <100%> (ø) ⬆️
src/components/Accordion/AccordionTitle.tsx 65% <100%> (+1.84%) ⬆️
src/components/Button/Button.tsx 95.83% <100%> (+0.08%) ⬆️
src/components/Icon/Icon.tsx 84% <100%> (+0.66%) ⬆️
src/components/Text/Text.tsx 100% <100%> (ø) ⬆️
src/components/Header/Header.tsx 94.73% <100%> (+0.29%) ⬆️
src/components/Header/HeaderDescription.tsx 100% <100%> (ø) ⬆️
... and 27 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 6493ae4...6d2bc87. Read the comment docs.

@layershifter
Copy link
Member

It seems that we can drop recast from dependencies, too.

@layershifter
Copy link
Member

We will also need to correct some types "before" | "after". However, it looks promising, nice job 👍

# Conflicts:
#	src/components/Text/Text.tsx
package.json Outdated
@@ -91,6 +91,7 @@
"lodash": "^4.17.10",
"prop-types": "^15.6.1",
"react-custom-scrollbars": "^4.2.1",
"react-docgen-typescript": "^1.12.0",
Copy link
Member

Choose a reason for hiding this comment

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

Oops, this should be in devDependencies

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the catch! Moved.

-moving react-docgen-typescript to devDependencies
@mnajdova
Copy link
Contributor Author

mnajdova commented Nov 9, 2018

I also noticed that some Component are missing some common properties:
AccordionContent

  • styles, variables

AccordionTitle

  • styles, variables

Attachment

  • className

Popup (maybe these shouldn't even be added, but just in case...)

  • as
  • styles
  • variables

Portal (maybe these shouldn't even be added, but just in case...)

  • as
  • className
  • styles
  • variabels

Segment

  • children (it's not added in props, but it is actually used)

Slot

  • children (it's not added in props, but it is actually used)

Text

  • children (it's not added in props, but it is actually used)

Using some common interface for these would help us avoiding some of these mistake, plus we would have consistent description for the common properties without having to copy paste the same thing, or change it everywhere if we decide for better description.

@bmdalex bmdalex mentioned this pull request Nov 14, 2018
6 tasks
manajdov added 2 commits November 14, 2018 14:08
# Conflicts:
#	src/components/Chat/ChatItem.tsx
#	src/components/Chat/ChatMessage.tsx
#	src/components/Segment/Segment.tsx
#	src/components/Slot/Slot.tsx
@mnajdova mnajdova merged commit 4986628 into master Nov 14, 2018
@mnajdova
Copy link
Contributor Author

Future work:

  • Had to add the accessibility defaultProps everywhere with a @default - we have to find alternative to this
  • Avatar – getInitials default value is not shown
  • ItemLayout render*** (some bugs even on prod) - default value not shown
  • Label accessibility was just added without description and default (didn't change anything there)
  • Portal context default is not shown
  • Popup content type has to be checked again
  • Variables type is any - there is interface for this but is not shown.

@layershifter layershifter deleted the feat/reusing-common-proptypes branch January 10, 2019 11:26
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