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

[docs] Generate proptypes from typescript demos #16521

Merged
merged 20 commits into from
Jul 18, 2019
Merged

Conversation

merceyz
Copy link
Member

@merceyz merceyz commented Jul 8, 2019

  • Generates proptypes for the javascript demos from the typescript version

Follow-up to #15438 (comment)

@mui-pr-bot
Copy link

mui-pr-bot commented Jul 8, 2019

Details of bundle changes.

Comparing: 8d8a8dd...50b57dd

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core 0.00% 0.00% 327,435 327,435 90,317 90,317
@material-ui/core/Paper 0.00% 0.00% 68,477 68,477 20,410 20,410
@material-ui/core/Paper.esm 0.00% 0.00% 61,761 61,761 19,177 19,177
@material-ui/core/Popper 0.00% 0.00% 28,896 28,896 10,394 10,394
@material-ui/core/Textarea 0.00% 0.00% 5,505 5,505 2,362 2,362
@material-ui/core/TrapFocus 0.00% 0.00% 3,753 3,753 1,577 1,577
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 16,156 16,156 5,816 5,816
@material-ui/core/useMediaQuery 0.00% 0.00% 3,098 3,098 1,310 1,310
@material-ui/lab 0.00% 0.00% 141,740 141,740 43,809 43,809
@material-ui/styles 0.00% 0.00% 51,886 51,886 15,380 15,380
@material-ui/system 0.00% 0.00% 15,576 15,576 4,445 4,445
Button 0.00% 0.00% 79,713 79,713 24,354 24,354
Modal 0.00% 0.00% 14,548 14,548 5,102 5,102
Portal 0.00% 0.00% 3,471 3,471 1,568 1,568
Rating 0.00% 0.00% 70,267 70,267 22,068 22,068
Slider 0.00% 0.00% 75,098 75,098 23,309 23,309
colorManipulator 0.00% 0.00% 3,904 3,904 1,543 1,543
docs.landing 0.00% 0.00% 54,338 54,338 13,762 13,762
docs.main 0.00% 0.00% 647,615 647,631 204,234 204,236
packages/material-ui/build/umd/material-ui.production.min.js 0.00% 0.00% 299,850 299,850 86,091 86,091

Generated by 🚫 dangerJS against 50b57dd

@joshwooding joshwooding added docs Improvements or additions to the documentation new feature New feature or request labels Jul 8, 2019
@eps1lon
Copy link
Member

eps1lon commented Jul 9, 2019

Source of truth should be typescript but you can be less strict most of the time e.g. you can used PropTypes.object over some complex PropTypes.shape.

Formatting? (prettier compacts small objects)

Prettier is used for formatting not our personal opinion. So if it does compact small objects then we stick with it.

About future work: Do you think we could make this work for our own components? It's been a long term goal to move props documentation to the .d.ts files (for in-editor docs) and generate docs and propTypes from it.

@merceyz
Copy link
Member Author

merceyz commented Jul 9, 2019

Prettier is used for formatting not our personal opinion. So if it does compact small objects then we stick with it.

I'm aware, just pointing out that they were not compact before.
Reason: Babel removes line info when I inject the proptypes, so prettier has nothing to go off

About future work: Do you think we could make this work for our own components?

Yes, might work already if the definition is changed to look like a function, otherwise it will need some work
export default function Button(props: type): JSX.Element

package.json Outdated Show resolved Hide resolved
@merceyz merceyz marked this pull request as ready for review July 9, 2019 14:25
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Really hyped about this for core components. Just some minor polish needed.

docs/src/pages/components/hidden/BreakpointDown.js Outdated Show resolved Hide resolved
docs/src/pages/components/tabs/NavTabs.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@oliviertassinari
Copy link
Member

@merceyz I have heard feedback from TypeScript users behind confused by the presence of the prop-types in the demos. I'm really happy to see this problem worked on, you know how to pick your fights 👍.

@merceyz
Copy link
Member Author

merceyz commented Jul 10, 2019

That should cover it for now.

I can update it to be more detailed, if that's wanted.

@merceyz
Copy link
Member Author

merceyz commented Jul 13, 2019

@eps1lon @oliviertassinari Is this too much? Feels like overkill.
5ba34d0

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 13, 2019

@merceyz withStyles has its own runtime checks to warn against wrong input. 👍 for keeping PropTypes.object. As for the other prop "object to shape" migrations. I would say that yes, it hits the point of diminishing returns. We have disabled https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/forbid-prop-types.md in our eslint config file. Not using shape would be aligned with it.

@eps1lon
Copy link
Member

eps1lon commented Jul 13, 2019

Yeah it's overkill for classes. I think making it configurable with a simple allow-list would be sufficient for a start. As far as I know any classes prop is injected from withStyles. We should be able to safely skip it by prop name.

@merceyz
Copy link
Member Author

merceyz commented Jul 13, 2019

So the requested changes are

  • Omit props with the name ref
  • Don't use shape for props with the name classes
    ?

@eps1lon
Copy link
Member

eps1lon commented Jul 13, 2019

Omit props with the name ref

Yes, always.

Don't use shape for props with the name classes

Yes, preferably via explicit configuration.

@merceyz
Copy link
Member Author

merceyz commented Jul 15, 2019

@eps1lon Done

@merceyz merceyz mentioned this pull request Jul 16, 2019
1 task
@eps1lon eps1lon merged commit 2c5d99e into mui:master Jul 18, 2019
@eps1lon
Copy link
Member

eps1lon commented Jul 18, 2019

Nicely done. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants