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.

Sign up for GitHub

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

Jump to bottom

[docs] Require documentation of demos #25811

Merged
merged 14 commits into from
Apr 23, 2021
Merged

[docs] Require documentation of demos #25811

merged 14 commits into from
Apr 23, 2021

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Apr 17, 2021 •
edited
Loading

Failing CI is expected since this change requires #25812

API documentation is one part of the documentation story. But documentation is only complete with demos and API.

yarn docs:api will now throw if a component has no demo listed:

$ yarn docs:api
Error: packages/material-ui-lab/src/StaticTimePicker/StaticTimePicker.tsx: Unable to find demos. 
Be sure to include `components: StaticTimePicker` in the markdown pages where the `StaticTimePicker` component is relevant. Every public component should have a demo. 
    at buildDocs (/home/eps1lon/Development/projects/mui/fork/docs/scripts/buildApi.ts:1021:11)
    at /home/eps1lon/Development/projects/mui/fork/docs/scripts/buildApi.ts:1372:14

This change touches older code so I used the opportunity to reduce technical debt (e.g. CJS -> ES -> TypeScript).

@eps1lon eps1lon added the docs Improvements or additions to the documentation label Apr 17, 2021
@mui-pr-bot
Copy link

mui-pr-bot commented Apr 17, 2021 •
edited
Loading

No bundle size changes

Generated by 🚫 dangerJS against e90efa6

@github-actions github-actions bot mentioned this pull request Apr 17, 2021
Closed
eps1lon added 8 commits April 17, 2021 11:34
@eps1lon
[core] Type-check yarn docs:i18n
cc050c6
@eps1lon
Ensure PageContext is fully typed
67ba55c 
JSDOC had a syntax error
@eps1lon
full type coverage for pageToTitleI18n
0ab376d 
Not automatically checked but can now be traced manually.
@eps1lon
Simplify pageToTitle implementation
b23b33d 
type-checked
@eps1lon
Revert "Simplify pageToTitle implementation"
a87a323 
This reverts commit b23b33d.

Let's do this another time to be safe i.e. make sure bisect, blame etc
are useful after squash+merge.
@eps1lon
Consolidate collection of demos
bd58f67
@eps1lon
yarn docs:api: Reduce noise in error reporting
0b64187
@eps1lon
Throw if no demos are found
352283c
@eps1lon eps1lon mentioned this pull request Apr 17, 2021
Merged
4 tasks
eps1lon
eps1lon commented Apr 17, 2021
View reviewed changes
docs/pages/api-docs/badge-unstyled.json Outdated
@@ -63,7 +63,7 @@
"forwardsRefTo": "HTMLSpanElement",
"filename": "/packages/material-ui-unstyled/src/BadgeUnstyled/BadgeUnstyled.js",
"inheritance": null,
"demos": "<ul><li><a href=\"/components/badges/\">Badges</a></li></ul>",
"demos": "<ul><li><a href=\"/components/avatars/\">Avatars</a></li>\n<li><a href=\"/components/badges/\">Badges</a></li></ul>",
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: Is the addition of Avatar here intended?

Copy link
Member Author

Choose a reason for hiding this comment

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

@mnajdova I guess the idea was to only list the demo of the styled component? My question would be why we special case the unstyled component in the first place i.e. just list BadgeUnstyled in /components/badges?

Copy link
Member

Choose a reason for hiding this comment

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

BadgeUnstyled is only referenced on the badge demo page. I fail to see the link with the avatar, outside of this demo: https://next.material-ui.com/components/avatars/#with-badge. It looks like a regression.

Copy link
Member Author

Choose a reason for hiding this comment

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

But at the same time it is already listed in the JSDOC for the Badge:
https://github.com/mui-org/material-ui/blob/b8bb45119ce0a2f97b70e361a4ac7179f6f8ad1a/packages/material-ui-unstyled/src/BadgeUnstyled/BadgeUnstyled.d.ts#L127-L128

Opening a separate PR to explicitly list these demos instead of trying to be too clever about it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Resolved by #25900

eps1lon
eps1lon commented Apr 17, 2021
View reviewed changes
docs/scripts/buildApi.ts
@@ -1372,7 +1378,7 @@ async function run(argv: {
workspaceRoot,
});
} catch (error) {
error.message = `${component.filename}: ${error.message}`;
error.message = `${path.relative(process.cwd(), component.filename)}: ${error.message}`;
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think the full path is relevant. Error message is now smaller.

The displayed path is still interpreted by VSCode as a path and therefore handled as a link.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 20, 2021
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 20, 2021
@eps1lon eps1lon marked this pull request as ready for review April 21, 2021 09:45
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 23, 2021
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 23, 2021
@eps1lon eps1lon merged commit e03ffd9 into mui:next Apr 23, 2021
@eps1lon eps1lon deleted the docs/mandatory-demo branch April 23, 2021 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliviertassinari oliviertassinari oliviertassinari left review comments

Assignees
No one assigned
Labels
docs Improvements or additions to the documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eps1lon @mui-pr-bot @oliviertassinari