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

docs(Props): improve table with props #1634

Merged
merged 12 commits into from
Jul 17, 2019
Merged

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented Jul 15, 2019

BREAKING CHANGES

  1. LoaderPosition is no longer exported.
// BEFORE
const position: LoaderPosition
// AFTER 
const position: LoaderProps['position']

Problem

To generate proper playgrounds for components we need to have a set of values props. It's impossible to get them from strings.

Allow to use Markdown in props description

This will allow us provide more meaningful descriptions for props.

Missing default values

Only values that were specified by @default tag in JSX were present, for Loader we have missed as, delay, labelPosition and size 💥

✅ Fixed by update of parseDefaultValue:

image

Unable to manipulate values

Previously we had just string as values ("gap.smaller" | "gap.small" | "gap.medium" | "gap.large") from react-typescript-docgen, so we can't to use them for playground generation.

✅ An updated parseType() function powered by Babel parses these strings and create proper definition structure:

    {
      "types": [
        {"value": "gap.smaller"},
        {"value": "gap.small"},
        {"value": "gap.medium"},
        {"value": "gap.large"}
      ],
      "name": "gap",
    },

Why use Babel in parseType()?

Only AST parser can provide proper output. Any implementation based on regexps will be fragile on things like generic types.

Why not use only Babel?

It will be hard to resolve types that are coming interfaces CommonProps, i.e. Babel good at parse, but it can't resolve TS types.

Handle edge cases

react-typescript-docgen uses typeToString() to generate strings with types mentioned above. Sometimes it generates things like this:
image

ℹ️ I tried to configure output of typeToString() manually via 3rd param (TypeFormatFlags), but it still resolved types.
✅ Fixed by direct lookup in Babel AST, see getTypeFromBabelTree()

Wrong as prop definition

It was marked as required and missed description.

image

🆚

image

Hard to understand what props can be passed to slot

image

What can be passed to clearIndicator?

image

Based on #1605 we know which props are used. With parsed types we can obviously understand relationship between components. In this example, BoxProps is a link to Box component 🚀

@codecov
Copy link

codecov bot commented Jul 15, 2019

Codecov Report

Merging #1634 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1634   +/-   ##
=======================================
  Coverage   71.28%   71.28%           
=======================================
  Files         852      852           
  Lines        7045     7045           
  Branches     2008     2029   +21     
=======================================
  Hits         5022     5022           
  Misses       2017     2017           
  Partials        6        6
Impacted Files Coverage Δ
packages/react/src/components/List/ListItem.tsx 100% <ø> (ø) ⬆️
packages/react/src/components/Menu/MenuDivider.tsx 100% <ø> (ø) ⬆️
packages/react/src/components/Popup/Popup.tsx 66.44% <ø> (ø) ⬆️
packages/react/src/components/Image/Image.tsx 100% <ø> (ø) ⬆️
packages/react/src/components/Slider/Slider.tsx 81.57% <ø> (ø) ⬆️
packages/react/src/components/Embed/Embed.tsx 94.44% <ø> (ø) ⬆️
...eact/src/lib/accessibility/FocusZone/FocusZone.tsx 87.79% <ø> (ø) ⬆️
packages/react/src/components/Tree/TreeItem.tsx 75% <ø> (ø) ⬆️
packages/react/src/components/Chat/ChatMessage.tsx 84.61% <ø> (ø) ⬆️
packages/react/src/components/Chat/Chat.tsx 62.5% <ø> (ø) ⬆️
... and 23 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 951b3f2...e5964a2. Read the comment docs.

@vercel vercel bot temporarily deployed to staging July 15, 2019 17:21 Inactive
name: 'componentInfo',
}),
)
.pipe(cache(gulpReactDocgen(['DOMAttributes', 'HTMLAttributes']), { name: 'componentInfo-1' }))
Copy link
Member Author

Choose a reason for hiding this comment

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

Should be increased otherwise everyone should clear cache manually

Copy link
Collaborator

Choose a reason for hiding this comment

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

don't we need to change .gitignore as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, this cache is stored in your TMP directory

Copy link
Member

@miroslavstastny miroslavstastny left a comment

Choose a reason for hiding this comment

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

Shorthand docs and typings not correct for collection shorthand with kind

Let's take ToolbarProps.items as an example:

  • it accepts kind attribute with 5 different values, one of them being default
  • based on the kind it calls shorthand factory for 4 different components.

But documentation just states ShorthandCollection<ToolbarItemProps> - it is missing information about kind and respective props.
Also the typing is not correct (though ObjectOf<any> "saves" that).

Sorry that I have not realised it when reviewing #1605 :-/

Link from ShorthandCollection<SubComponent> goes to SubComponent page

If you follow the link from ShorthandCollection<ToolbarItemProps> you will get to ToolbarItemProps page - which correctly describes the props but other than that looks like a dead page (no examples...).
I think I can confuse consumers, it should rather link to parent component page with subcomponent props opened.

* A slider can be read-only and unable to change states.
* @default false
*/
/** A slider can be read-only and unable to change states. */
Copy link
Member

Choose a reason for hiding this comment

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

I understand @default is no longer used by docsite, but it is still used by intellisense.
Do we want to remove it or rather test that it is present and correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's hard to maintain things like these because they don't aligned with code. I think that the best way is to remove them and rely on defaultProps.

@@ -15,19 +15,14 @@ import { Accessibility } from '../../lib/accessibility/types'
import { WithAsProp, ShorthandValue, withSafeTypeForAs } from '../../types'
import Box, { BoxProps } from '../Box/Box'

export type LoaderPosition = 'above' | 'below' | 'start' | 'end'
Copy link
Member

Choose a reason for hiding this comment

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

Removal of this makes it a BREAKING CHANGE. Is it necessary?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't @layershifter just inlining this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Technically it's breaking change, will mention this in CHANGELOG and in a PR header.
It's necessary to avoid manual type definition for autogenerated playground.

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh, we were exporting this type 🤦‍♂ great catch @miroslavstastny

Copy link
Collaborator

@bmdalex bmdalex left a comment

Choose a reason for hiding this comment

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

looks good to me, few comments; awesome job 👍

build/gulp/plugins/util/getComponentInfo.ts Show resolved Hide resolved
build/gulp/plugins/util/getShorthandInfo.ts Show resolved Hide resolved
name: 'componentInfo',
}),
)
.pipe(cache(gulpReactDocgen(['DOMAttributes', 'HTMLAttributes']), { name: 'componentInfo-1' }))
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't we need to change .gitignore as well?

@@ -15,19 +15,14 @@ import { Accessibility } from '../../lib/accessibility/types'
import { WithAsProp, ShorthandValue, withSafeTypeForAs } from '../../types'
import Box, { BoxProps } from '../Box/Box'

export type LoaderPosition = 'above' | 'below' | 'start' | 'end'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't @layershifter just inlining this?

@vercel vercel bot requested a deployment to staging July 16, 2019 12:12 Abandoned
@vercel vercel bot requested a deployment to staging July 16, 2019 13:08 Abandoned
@vercel vercel bot requested a deployment to staging July 16, 2019 13:30 Abandoned
@vercel vercel bot requested a deployment to staging July 16, 2019 13:42 Abandoned
@vercel vercel bot requested a deployment to staging July 16, 2019 13:43 Abandoned
@layershifter
Copy link
Member Author

@miroslavstastny great catches, fixed them in 9956115 and 83c1577 👍

@vercel vercel bot temporarily deployed to staging July 17, 2019 10:28 Inactive
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.

6 participants