Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better prop-types definitions with flow types #781

Merged
merged 6 commits into from
Mar 20, 2018

Conversation

nanot1m
Copy link
Contributor

@nanot1m nanot1m commented Jan 20, 2018

Some plans:

  • handle rendering of functions
  • handle rendering of arrays
  • handle rendering of shapes
  • handle rendering of union-types

@nanot1m
Copy link
Contributor Author

nanot1m commented Jan 20, 2018

Current result
2018-01-20 14 19 41

@nanot1m
Copy link
Contributor Author

nanot1m commented Jan 20, 2018

Should complex types (like shapes, or function) be rendered in the type column or in the description column?

@codecov-io
Copy link

codecov-io commented Jan 20, 2018

Codecov Report

Merging #781 into master will increase coverage by 0.18%.
The diff coverage is 95.83%.

Impacted Files Coverage Δ
src/rsg-components/Props/PropsRenderer.js 98.27% <95.83%> (+2.35%) ⬆️

@nanot1m nanot1m mentioned this pull request Jan 20, 2018
@sapegin
Copy link
Member

sapegin commented Jan 20, 2018

Good start, thanks!

I think we should render them in tooltips like default values on your screenshot, they may be very large.

@nanot1m
Copy link
Contributor Author

nanot1m commented Jan 21, 2018

Found a problem with react-docgen: it doesn't handle array types marked with [].

For example, type foo: string[] would be parsed as unknown type

@nanot1m
Copy link
Contributor Author

nanot1m commented Jan 21, 2018

Implemented rendering complex types in tooltip
2018-01-21 14 18 55

Do you have any thoughts about how to improve rendering?

@nanot1m nanot1m changed the title [WIP] Better prop-types definitions with flow types Better prop-types definitions with flow types Jan 21, 2018
@nanot1m
Copy link
Contributor Author

nanot1m commented Jan 21, 2018

I'm going to remove FlowButton component from basic example

@sapegin
Copy link
Member

sapegin commented Jan 22, 2018

Do you have any thoughts about how to improve rendering?

I think it's good enough.

Copy link
Member

@sapegin sapegin left a comment

Choose a reason for hiding this comment

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

Couple of small remarks ;-)

* @returns {boolean}
*/
export function isFlowTyped(prop) {
return Boolean(prop.flowType);
Copy link
Member

Choose a reason for hiding this comment

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

Can't we check prop.flowType directly? (You're doing it anyway ;–) This function feels unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just abstraction, and it can be safely thrown away

Copy link
Member

Choose a reason for hiding this comment

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

There's no such thing as "just an abstraction" ;-) each has its own cost. I don't think this one worth its cost, and since you're not using it consistently, which makes the cost even higher.

function renderComplexType(name, title) {
return (
<Text size="small" underlined title={title}>
<Type>{name}</Type>
Copy link
Member

Choose a reason for hiding this comment

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

This whole thing is already wrapped in <Type>, is it necessary to have one more here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Text component does not inherit color from parents. Should we add a prop color="inherit" to Text?

Copy link
Member

Choose a reason for hiding this comment

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

OK, leave it like this for now ;-)

@nanot1m
Copy link
Contributor Author

nanot1m commented Jan 22, 2018

I've forgotten about public components methods. Have to update them too

@nanot1m
Copy link
Contributor Author

nanot1m commented Jan 22, 2018

Maybe public methods should be updated in another pull request? If so, i'm done with this task. And will continue work in another brunch

@sapegin
Copy link
Member

sapegin commented Jan 22, 2018

Good idea!

@sapegin
Copy link
Member

sapegin commented Jan 22, 2018

Could you take a look at the CI log here?

@nanot1m
Copy link
Contributor Author

nanot1m commented Jan 23, 2018

Error: Navigation Timeout Exceeded: 30000ms exceeded
    at Promise.then (/home/travis/build/styleguidist/react-styleguidist/node_modules/puppeteer/lib/NavigatorWatcher.js:71:21)

Something went wrong with puppeteer.
Can you restart CI task?

@nanot1m
Copy link
Contributor Author

nanot1m commented Jan 23, 2018

❌ codecov/patch — 95.83% of diff hit (target 96.33%)

Can you explain, what does this mean? And how can i improve code coverage?

@sapegin
Copy link
Member

sapegin commented Jan 23, 2018

codecov/patch — 95.83% of diff hit (target 96.33%)

It means that the test coverage for the lines you changed in this PR is 95.83% and recommends to add more test to match the goal of 96.33% (I'm not sure where it's coming from). You can click Details and see which lines aren't covered by tests.

@nanot1m
Copy link
Contributor Author

nanot1m commented Jan 23, 2018

Thanks. I've looked at uncovered places. There are two uncovered lines, which are not referenced to my changes.

one is

export function getRowKey(row) {
  return row.name;
}

and another is

function renderEnum(prop) {
  if (!Array.isArray(getType(prop).value)) {
    return <span>{getType(prop).value}</span>; // not hitting this line
  }
...

Should i write tests for getRowKey?

@sapegin
Copy link
Member

sapegin commented Jan 23, 2018

That would be supercool but feel free to ignore them ;-)

@nanot1m
Copy link
Contributor Author

nanot1m commented Jan 23, 2018

I've added test, nothing changed (only codecov/project improved). So let's skip this warning 👍

@sapegin sapegin added this to the 6.3.0 milestone Jan 25, 2018
@sapegin
Copy link
Member

sapegin commented Jan 25, 2018

Thanks, I'll merge it a bit later -- don't want to block master because we have a few unfixed bugs.

@sapegin sapegin merged commit 5ef15c8 into styleguidist:master Mar 20, 2018
@sapegin
Copy link
Member

sapegin commented Mar 20, 2018

🎉 This PR is included in version 6.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

sapegin pushed a commit that referenced this pull request Mar 22, 2018
👋 **[Support Styleguidist](https://opencollective.com/styleguidist) on Open Collective** 👋

## New features

### Page per section 🚧

This is a huge improvement for large style guides: now you can show only one component at a page instead of all components.

![](https://d3vv6lp55qjaqc.cloudfront.net/items/0W2q2K2s3n2k311O1J0y/Screen%20Recording%202018-03-22%20at%2010.06%20AM.gif)

To enable:

```js
module.exports = {
  pagePerSection: true
};
```

This is an experimental feature and we need your feedback to make it better and really useful. For example, right now there’s no isolated mode. So feel free to share your ideas [in issues](https://github.com/styleguidist/react-styleguidist/issues).

(#835 by @bitionaire and @stepancar, closes #494 and #768)

### “Fork me” ribbon

One more step to make Styleguidist usable for documenting open source projects:

![](https://d3vv6lp55qjaqc.cloudfront.net/items/1t331n2a3v0F2i290K0Z/Image%202018-03-22%20at%209.13.11%20AM.png)

New config option to enable the ribbon, define the URL and change the label:

```js
module.exports = {
  ribbon: {
    url: 'http://example.com/',
    text: 'Fork me on GitHub'
  }
};
```

And two new [theme variables](https://github.com/styleguidist/react-styleguidist/blob/master/src/styles/theme.js) to change colors: `color.ribbonBackground` and `color.ribbonText`.

(#861 by @glebez and @wkwiatek, part of #195, closes #647)

### Options to change CLI output on server start and build

Two new options, `printServerInstructions` and `printBuildInstructions`:

```js
module.exports = {
  printBuildInstructions(config) {
    console.log(
      `Style guide published to ${
        config.styleguideDir
      }. Something else interesting.`
    );
  }
};
```

(#878 by @roblevintennis, closes #876)

### Option to modify props

A new option, `updateDocs` to modify props before rendering. For example, you can load a component version number from a JSON file:

```js
module.exports = {
  updateDocs(docs) {
    if (docs.doclets.version) {
      const versionFilePath = path.resolve(
        path.dirname(file),
        docs.doclets.version
      );
      const version = require(versionFilePath).version;

      docs.doclets.version = version;
      docs.tags.version[0].description = version;
    }

    return docs;
  }
};
```

(#868 by @ryanoglesby08)

### Limited support for named exports

Styleguidist used to require default exports for components. Now you can use named exports too, though Styleguidist still supports only one component per file. I hope this change will make some users happier, see more details [in the docs](https://react-styleguidist.js.org/docs/components.html#loading-and-exposing-components).

(#825 by @marcdavi-es, closes #820 and #633)

### Allow arrays of component paths in sections

More flexibility in structuring your style guide:

```js
module.exports = {
  sections: [
    {
      name: 'Forms',
      components: [
        'lib/components/ui/Button.js',
        'lib/components/ui/Input.js'
      ]
    }
  ]
};
```

(#794 by @glebez, closes #774)

### Sort properties by `required` and `name` attributes

This change should make props tables more predictable for the users. Instead of relying on developers to sort props in a meaningful way, Styleguidist will show required props first, and sort props in each group alphabetically.

To disable sorting, use the identity function:

```javascript
module.exports = {
  sortProps: props => props
};
```

(#784 by @dotcs)

## Bug fixes

* Allow `Immutable` state in examples (#870 by @nicoffee, closes #864)
* Change template container ID to prevent clashes (#859 by @glebez, closes #753)
* Better props definitions with Flow types (#781 by @nanot1m)
@SavePointSam
Copy link

Is there an expectation that this should render as a union?

const THEMES: {
  PRIMARY: 'primary',
  SECONDARY: 'secondary',
} = {
  PRIMARY: 'primary',
  SECONDARY: 'secondary',
};

type Props = {
  /** display theme */
  theme?: $Values<typeof THEMES>,
};

Flow just added support for this type of syntax with 0.68.0. I'm currently getting the following render in my styleguide.

image


On another note, is there work planned/being done to make the experience of hover details for something like a tuple better? Having the alt text show more information is ok, but I was confused at first as there wasn't something like tooltip.js appearing. It would also be nice to expand sub-types from that message too. For example I have a type that is an array of types and it just outputs [Action] which isn't clear to an end user. Maybe thats good enough though as a consumer of the API will be able to discover what the Action would be?

Thanks for this update! My styleguide is sooo much better documented out of the box. 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants