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] Batch small changes #32170

Merged
merged 6 commits into from May 30, 2022
Merged

Conversation

michaldudak
Copy link
Member

@michaldudak michaldudak added the docs Improvements or additions to the documentation label Apr 7, 2022
@mui-bot
Copy link

mui-bot commented Apr 7, 2022

No bundle size changes

Generated by 🚫 dangerJS against 8495395

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

👌

@@ -15,7 +15,7 @@ Some important features of the `Popper` component:

- 🕷 Popper relies on the 3rd party library ([Popper.js](https://popper.js.org/)) for perfect positioning.
- 💄 It's an alternative API to react-popper. It aims for simplicity.
- 📦 [8 kB gzipped](/size-snapshot).
- 📦 [17.3 kB gzipped](/size-snapshot).
Copy link
Member

Choose a reason for hiding this comment

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

? https://mui.com/size-snapshot

Screenshot 2022-04-07 at 17 47 04

Suggested change
- 📦 [17.3 kB gzipped](/size-snapshot).
- 📦 [24.9 kB gzipped](/size-snapshot).

I'm not sure if it even makes sense to have all these demos https://mui.com/components/popper/. Maybe simply linking the base page and saying it also has the sx prop would be better, no duplication of content, less confusion.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why does bundlephobia show 17.3, then?

I'm not sure if it even makes sense to have all these demos https://mui.com/components/popper/. Maybe simply linking the base page and saying it also has the sx prop would be better, no duplication of content, less confusion.

It's a hard call - on the other hand devs using just Material UI could expect docs for everything exported from @mui/material within the Material UI space, so duplication is justified.

Copy link
Member

Choose a reason for hiding this comment

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

Oh right, there is a 7.6 kB difference. Maybe emotion's related? AFAIK bundlephobia doesn't take peer dependencies into account, our bundle size scripts has externals: /^(date-fns|dayjs|luxon|moment|react|react-dom)(\/.*)?$/, in its webpack config. So https://mui.com/size-snapshot takes emotion into account.

Copy link
Member

Choose a reason for hiding this comment

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

It's a hard call - on the other hand devs using just Material UI could expect docs for everything exported from @mui/material within the Material UI space, so duplication is justified.

I would 👍 to keep the page but to strip down the content. Instead, we would teach developers the inheritance pattern. It would be similar to how Dialog extends Modal and we don't duplicate the content but cross link: https://mui.com/components/dialogs/#performance. I think it's much better as it makes having high-quality docs content possible (no duplication effort, which leads to more up-to-date, accurate, comprehensive, and consistent content). I would not work for all components, I think that it works well with Popper because it's mostly unstyled.

Copy link
Member Author

Choose a reason for hiding this comment

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

Instead, we would teach developers the inheritance pattern. It would be similar to how Dialog extends Modal and we don't duplicate the content but cross link: https://mui.com/components/dialogs/#performance. I think it's much better as it makes having high-quality docs content possible

It may be much better for us as docs authors, but I personally hated this pattern as a developer trying to learn about Material UI in the past.
When I'm learning about a component, I'd like to have all the meaningful information on one page. Having to jump between pages is counterproductive and sometimes confusing. Developers usually don't care (nor they should) that a given component inherits or uses another one internally. They are concerned about the external API.

Copy link
Member

Choose a reason for hiding this comment

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

I personally hated this pattern as a developer trying to learn about Material UI in the past.

I agree that it comes with its drawback. I would personally give more value to the pros of linking (e.g. teaching how things are inherited to developers) than the pros of duplication (I would fear leads to too much dilution of efforts; a lower quality doc overall for developers). I think that the best of the two would be this pattern: https://www.notion.so/help/synced-blocks, with a visible link to the source so we retain the "teaching of the inheritance structure" feature.

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Is this one correct too? #31813 (comment)

@michaldudak
Copy link
Member Author

Is this one correct too? #31813 (comment)

Good catch. Fixed.

@danilo-leal danilo-leal changed the title Batch small changes [docs] Batch small changes Apr 7, 2022
@danilo-leal danilo-leal added the package: base-ui Specific to @mui/base label Apr 7, 2022
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 8, 2022
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 12, 2022
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 27, 2022
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label May 27, 2022
@michaldudak
Copy link
Member Author

@oliviertassinari Could we merge this PR and discuss how we approach documenting components like Popper with the whole team?

@oliviertassinari
Copy link
Member

oliviertassinari commented May 27, 2022

@michaldudak Sure, we can move iteratively. The changes here are already a clear step forward.

discuss how we approach documenting components like Popper with the whole team?

It might actually deserves a notion page too. I think that we have the exact same problem with the system, for example this duplication

opens for questions cc @mui/core

@samuelsycamore
Copy link
Member

@oliviertassinari Could we merge this PR and discuss how we approach documenting components like Popper with the whole team?

This is a great question, and particularly relevant to the work I've been doing on the Base docs. Pages like Popper and Portal are now in better shape IMO in the Base docs vs the older Material UI docs, and I think we should migrate the newer drafts over.

I agree with @michaldudak that we should not make the developer jump between multiple pages to find all the info they need when dealing with a single component, as a general rule. That means we would err on the side of duplicating content. That's obviously not ideal for us as maintainers, but overall I think it's far preferable for the reader.

@oliviertassinari
Copy link
Member

oliviertassinari commented May 27, 2022

I think that we will need to broaden the scope of the problem. I think that the implications go beyond the tradeoff that we have discussed so far (the pain of jumping docs vs. content quality), it's also about teaching developers our different products. For example, when you use Radix, you are forced to learn about Stitches https://www.radix-ui.com/docs/primitives/components/dialog#examples. When you use Tailwind UI, they force you to learn Headless UI. Right now, we bundle everything in @mui/material and @mui/joy which feels like we train our audience to use these two packages or if their requirement changes, to use nothing. Maybe we have an opportunity to teach them to cheery picking what they need, maybe it's the key change that we are missing to crack the 50% adoption level in the React ecosystem problem (current 25%).

@samuelsycamore
Copy link
Member

I think that we will need to broaden the scope of the problem. I think that the implications go beyond the tradeoff that we have discussed so far (the pain of jumping docs vs. content quality), it's also about teaching developers our different products.

I see what you're getting at, and I agree that it makes sense from the perspective of educating our users on how to better use our product suite as a whole. It's a complex UX problem to solve.

@michaldudak michaldudak merged commit 84918a8 into mui:master May 30, 2022
@michaldudak michaldudak deleted the batch-small-changes branch May 30, 2022 08:41
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 package: base-ui Specific to @mui/base
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants