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] Add Shadow DOM guide #33007

Merged
merged 3 commits into from Jun 12, 2022
Merged

Conversation

cherniavskii
Copy link
Member

@cherniavskii cherniavskii commented Jun 3, 2022

@cherniavskii cherniavskii added the docs Improvements or additions to the documentation label Jun 3, 2022
@mui-bot
Copy link

mui-bot commented Jun 3, 2022

No bundle size changes

Generated by 🚫 dangerJS against d7928da

@cherniavskii cherniavskii marked this pull request as ready for review June 3, 2022 13:24
Copy link
Member

@samuelsycamore samuelsycamore left a comment

Choose a reason for hiding this comment

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

Solid addition! I've left some suggestions to flesh out the content a bit.

docs/data/material/guides/shadow-dom/shadow-dom.md Outdated Show resolved Hide resolved
docs/data/material/guides/shadow-dom/shadow-dom.md Outdated Show resolved Hide resolved

### 2. Theme

MUI components like `Menu`, `Dialog`, `Popover` and others use [React Portal](https://reactjs.org/docs/portals.html) to render a new "subtree" in a container outside of current DOM hierarchy.
Copy link
Member

Choose a reason for hiding this comment

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

Replacing MUI with Material UI to be more specific.

Should we replace this Portal link with our own Portal docs? (If so, we still need to update the Material UI portal docs to match the Base portal docs.)

Suggested change
MUI components like `Menu`, `Dialog`, `Popover` and others use [React Portal](https://reactjs.org/docs/portals.html) to render a new "subtree" in a container outside of current DOM hierarchy.
Material UI components like `Menu`, `Dialog`, `Popover` and others use [React Portal](https://reactjs.org/docs/portals.html) to render a new "subtree" in a container outside of current DOM hierarchy.

Copy link
Member Author

Choose a reason for hiding this comment

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

Replacing MUI with Material UI to be more specific.

I would keep MUI, since it's relevant for MUI X and probably for Joy (in the future)

Should we replace this Portal link with our own Portal docs?

Sure 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

I would keep MUI, since it's relevant for MUI X and probably for Joy (in the future)

More and more I'm getting hints for an MUI-company-wide documentation 👁️ 👀 🙄
(stuff that isn't particular to one or two products but rather a company-wide philosophy/convention/approach)

Copy link
Member Author

Choose a reason for hiding this comment

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

Huh, I didn't even realise that this guide will be only available in Material UI docs.
I think a lot of pages from "How To Guides" menu section could make sense in other MUI Core products, and sometimes in MUI X as well.
We'll definitely need top-level docs at some point.

### 2. Theme

MUI components like `Menu`, `Dialog`, `Popover` and others use [React Portal](https://reactjs.org/docs/portals.html) to render a new "subtree" in a container outside of current DOM hierarchy.
By default, this container is `document.body`. But since the styles are applied only inside of the Shadow DOM, we need to render portals inside the Shadow DOM container as well:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
By default, this container is `document.body`. But since the styles are applied only inside of the Shadow DOM, we need to render portals inside the Shadow DOM container as well:
By default, this container is `document.body`.
But since the styles are applied only inside of the shadow DOM, we need to render portals inside the shadow DOM container as well:

Copy link
Member Author

Choose a reason for hiding this comment

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

Newline doesn't make a difference in markdown.
Did you mean to split it into two paragraphs? An additional empty line is needed then

Copy link
Member

Choose a reason for hiding this comment

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

I like to see each sentence on a new line to make it simpler to comment on individual sentences (rather than paragraphs). Sorry I should have explained that! It's a pattern that I think I saw @oliviertassinari enforce when I started contributing to the docs. Might be worth adding as a prettier rule if it's something we really want to standardize.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, so you meant just a line break in markdown, right?

docs/data/material/guides/shadow-dom/shadow-dom.md Outdated Show resolved Hide resolved
docs/data/material/guides/shadow-dom/shadow-dom.md Outdated Show resolved Hide resolved
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

@cherniavskii great job and initiative for adding this guide 🙏

@cherniavskii cherniavskii merged commit 60eaa3b into mui:master Jun 12, 2022
@cherniavskii cherniavskii deleted the shadow-dom-guide branch June 12, 2022 11:33

In the example below you can see that the component outside of the shadow DOM is affected by global styles, while the component inside of the shadow DOM is not:

{{"demo": "ShadowDOMDemo.js", "defaultCodeOpen": false}}
Copy link
Member

Choose a reason for hiding this comment

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

I did a few changes around this demo in #33122

@@ -208,6 +208,7 @@ const pages = [
{ pathname: '/material-ui/guides/content-security-policy', title: 'Content Security Policy' },
{ pathname: '/material-ui/guides/right-to-left', title: 'Right-to-left' },
{ pathname: '/material-ui/guides/flow' },
Copy link
Member

Choose a reason for hiding this comment

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

I think that we could drop this page, it's outdated now. I have taken note in https://www.notion.so/mui-org/Opportunities-408a9771556a4715830d18f3749af753#167b4a8cef8e44e2a31c9c2568ec12fb.

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
Projects
None yet
6 participants