docs(Props): improve table with props by layershifter · Pull Request #1634 · microsoft/fluent-ui-react

layershifter · 2019-07-15T10:16:08Z

BREAKING CHANGES

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:

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:

ℹ️ 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.

🆚

Hard to understand what props can be passed to slot

What can be passed to clearIndicator?

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 · 2019-07-15T13:19:15Z

Codecov Report

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

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

layershifter · 2019-07-15T17:23:02Z

build/gulp/tasks/docs.ts

-        name: 'componentInfo',
-      }),
-    )
+    .pipe(cache(gulpReactDocgen(['DOMAttributes', 'HTMLAttributes']), { name: 'componentInfo-1' }))


Should be increased otherwise everyone should clear cache manually

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

Nope, this cache is stored in your TMP directory

miroslavstastny

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.

miroslavstastny · 2019-07-16T06:46:20Z

packages/react/src/components/Slider/Slider.tsx

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


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?

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.

miroslavstastny · 2019-07-16T06:49:28Z

packages/react/src/components/Loader/Loader.tsx

 import { WithAsProp, ShorthandValue, withSafeTypeForAs } from '../../types'
 import Box, { BoxProps } from '../Box/Box'

-export type LoaderPosition = 'above' | 'below' | 'start' | 'end'


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

Isn't @layershifter just inlining this?

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.

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

docs/src/components/ComponentDoc/ComponentPropsTable/ComponentPropsRow.tsx

bmdalex

looks good to me, few comments; awesome job 👍

build/gulp/plugins/util/getComponentInfo.ts

build/gulp/plugins/util/getShorthandInfo.ts

bmdalex · 2019-07-16T09:58:17Z

build/gulp/tasks/docs.ts

-        name: 'componentInfo',
-      }),
-    )
+    .pipe(cache(gulpReactDocgen(['DOMAttributes', 'HTMLAttributes']), { name: 'componentInfo-1' }))


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

docs/src/components/ComponentDoc/ComponentPropsTable/ComponentPropsRow.tsx

bmdalex · 2019-07-16T10:35:19Z

packages/react/src/components/Loader/Loader.tsx

 import { WithAsProp, ShorthandValue, withSafeTypeForAs } from '../../types'
 import Box, { BoxProps } from '../Box/Box'

-export type LoaderPosition = 'above' | 'below' | 'start' | 'end'


Isn't @layershifter just inlining this?

packages/react/src/lib/accessibility/FocusZone/AutoFocusZone.tsx

…com/stardust-ui/react into chore/prop-generator

layershifter · 2019-07-16T13:49:35Z

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

…com/stardust-ui/react into chore/prop-generator

layershifter added 🚧 WIP 💥 blocked 📚 docs labels Jul 15, 2019

vercel bot deployed to staging July 15, 2019 10:33 View deployment

mnajdova removed the 📚 docs label Jul 15, 2019

vercel bot deployed to staging July 15, 2019 12:17 View deployment

vercel bot deployed to staging July 15, 2019 13:05 View deployment

vercel bot had a problem deploying to staging July 15, 2019 13:42 Failure

vercel bot deployed to staging July 15, 2019 13:46 View deployment

chore: improve docgen

589d5f8

layershifter force-pushed the chore/prop-generator branch from 5011d3a to 589d5f8 Compare July 15, 2019 17:21

vercel bot temporarily deployed to staging July 15, 2019 17:21 Inactive

layershifter added 📚 docs and removed 🚧 WIP 💥 blocked labels Jul 15, 2019

layershifter commented Jul 15, 2019

View reviewed changes

remove changes in type

c34a800

vercel bot had a problem deploying to staging July 15, 2019 17:26 Failure

layershifter added the 🚀 ready for review label Jul 15, 2019

layershifter marked this pull request as ready for review July 15, 2019 17:27

layershifter requested review from dvdzkwsk, kuzhelov, levithomason, miroslavstastny and mnajdova as code owners July 15, 2019 17:27

fix types

8fc9589

vercel bot deployed to staging July 15, 2019 17:30 View deployment

miroslavstastny reviewed Jul 16, 2019

View reviewed changes

alinais approved these changes Jul 16, 2019

View reviewed changes

bmdalex approved these changes Jul 16, 2019

View reviewed changes

bmdalex force-pushed the chore/prop-generator branch from 8fc9589 to 05430cb Compare July 16, 2019 12:11

vercel bot requested a deployment to staging July 16, 2019 12:12 Abandoned

layershifter force-pushed the chore/prop-generator branch from 05430cb to 8fc9589 Compare July 16, 2019 12:26

layershifter added 3 commits July 16, 2019 14:31

add changelog entry

4eb4c6c

fix review comment

e15ad2f

fix review comment: improve code

0bd3449

vercel bot requested a deployment to staging July 16, 2019 13:08 Abandoned

fix review comment: fix handling of ShorthandCollection

83c1577

vercel bot requested a deployment to staging July 16, 2019 13:30 Abandoned

fix handling of component links

9956115

vercel bot requested a deployment to staging July 16, 2019 13:42 Abandoned

Merge branches 'chore/prop-generator' and 'master' of https://github.…

19ba052

…com/stardust-ui/react into chore/prop-generator

vercel bot requested a deployment to staging July 16, 2019 13:43 Abandoned

Merge branch 'master' into chore/prop-generator

6f0f129

vercel bot deployed to staging July 16, 2019 16:28 View deployment

layershifter mentioned this pull request Jul 17, 2019

ShorthandCollection does not contain proper definition #1664

Open

Merge branches 'chore/prop-generator' and 'master' of https://github.…

6ae5737

…com/stardust-ui/react into chore/prop-generator

vercel bot temporarily deployed to staging July 17, 2019 10:28 Inactive

update changelog

e5964a2

vercel bot deployed to staging July 17, 2019 10:30 View deployment

miroslavstastny approved these changes Jul 17, 2019

View reviewed changes

layershifter merged commit a5f2f80 into master Jul 17, 2019

delete-merged-branch bot deleted the chore/prop-generator branch July 17, 2019 10:51

assuncaocharles mentioned this pull request May 9, 2020

fix(Types): ShorthandCollection microsoft/fluentui#13050

Merged

2 tasks

layershifter mentioned this pull request Aug 12, 2021

feat(react-conformance): add test for callback arguments microsoft/fluentui#19369

Merged

2 tasks

Conversation

layershifter commented Jul 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

BREAKING CHANGES

Problem

Allow to use Markdown in props description

Missing default values

Unable to manipulate values

Why use Babel in parseType()?

Why not use only Babel?

Handle edge cases

Wrong as prop definition

Hard to understand what props can be passed to slot

Uh oh!

codecov bot commented Jul 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miroslavstastny left a comment

Choose a reason for hiding this comment

Shorthand docs and typings not correct for collection shorthand with kind

Link from ShorthandCollection<SubComponent> goes to SubComponent page

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bmdalex left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

layershifter commented Jul 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

layershifter commented Jul 15, 2019 •

edited

Loading

Why use Babel in `parseType()`?

Wrong `as` prop definition

codecov bot commented Jul 15, 2019 •

edited

Loading

Shorthand docs and typings not correct for collection shorthand with `kind`

Link from `ShorthandCollection<SubComponent>` goes to SubComponent page