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 theme.applyStyles and migrate docs #42498

Merged
merged 33 commits into from
Jun 11, 2024

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Jun 2, 2024

Docs update

Before:

After:

The recommended approach for styling dark mode is to use theme.applyStyles('dark', …) and stop using theme.palette.mode === 'dark' check to smoothly migrate to Pigment CSS.

This PR includes the migration of the docs using codemods with manual adjustments. The changes are huge but I don't think we can avoid it.


@siriwatknp siriwatknp added the docs Improvements or additions to the documentation label Jun 2, 2024
@mui-bot
Copy link

mui-bot commented Jun 2, 2024

@zannager zannager requested a review from zanivan June 3, 2024 15:02
Comment on lines 21 to 32
sx={[
{
overflow: 'auto',
},
flexBasis
? {
flexBasis: `${flexBasis}px`,
}
: {
flexBasis: null,
},
]}
Copy link
Contributor

Choose a reason for hiding this comment

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

Are all of these sx ternary stuff related to the PR's goal or should we keep it separate? Also, I'm curious why this approach is better than the previous one 🤔

Copy link
Member Author

@siriwatknp siriwatknp Jun 4, 2024

Choose a reason for hiding this comment

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

Ideally, it can be a separate PR. It was updated due to the sx codemod (also transforming theme.palette.mode condition).

I'm curious why this approach is better than the previous one 🤔

It's compatible with Pigment CSS. If you don't mind, I can update the sx prop docs in this PR altogether.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah cool! I'm slightly sad as this syntax is way uglier than the previous one 😅 but it feels all right to update the docs in a dedicated PR. Maybe it's worth highlighting these things (why these ternaries were picked up and missing tasks) in the PR's description, though!

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 4, 2024
@@ -31,14 +31,12 @@ export default function ExampleCollapsibleList() {
'--joy-palette-text-secondary': '#635e69',
'--joy-palette-primary-plainColor': '#d48cff',
},

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 a saw a PR for fix this, but in case it was unrelated, we shouldn't transform these lines.

@siriwatknp
Copy link
Member Author

siriwatknp commented Jun 9, 2024

I haven't tested the changes by browsing the docs, but we certanly should do it before merging, e.g. I noticed a bug on the landing page immediatelly (Move faster text has the wrong color):

This is fixed, I missed the default color primary.

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.

I noticed we are missing the current item being highlighted in the Table of contents in both light and dark mode, this is how it looks like on master (the Sizes item being highlighted):

Screenshot 2024-06-10 at 11 53 01 Screenshot 2024-06-10 at 11 54 05

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 10, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 11, 2024
@siriwatknp
Copy link
Member Author

I noticed we are missing the current item being highlighted in the Table of contents in both light and dark mode, this is how it looks like on master (the Sizes item being highlighted):

Screenshot 2024-06-10 at 11 53 01 Screenshot 2024-06-10 at 11 54 05

Can you check again, it looks normal to me.

image

@siriwatknp siriwatknp requested a review from mnajdova June 11, 2024 04:46
@mnajdova
Copy link
Member

Can you check again, it looks normal to me.

Looks good on the latest netlify build, not sure what was the issue.

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.

I can't find anything else, I think we can merge and fix any regression in case we missed something. Awesome job Jun, both on the codemod and the migration 🤩

@siriwatknp siriwatknp enabled auto-merge (squash) June 11, 2024 10:45
@siriwatknp siriwatknp merged commit 219380c into mui:next Jun 11, 2024
18 checks passed
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Jun 12, 2024
@danilo-leal
Copy link
Contributor

Heya! @siriwatknp found a place where it seems like the new way to write conditional styles isn't working properly: https://next--material-ui.netlify.app/experiments/docs/demos/ — the toolbar borderRadius: 0 is not being applied when openDemoSource is true (see how there is border radius on them). Similarly with the demoOptions condition.

    variants: [
      {
        props: ({ demoOptions }) => demoOptions.bg === 'inline',
        style: {
          marginTop: theme.spacing(1),
          borderTopWidth: 1,
        },
      },
      {
        props: ({ openDemoSource }) => openDemoSource,
        style: {
          borderRadius: 0,
        },
      },
    ],

@siriwatknp
Copy link
Member Author

Heya! @siriwatknp found a place where it seems like the new way to write conditional styles isn't working properly: https://next--material-ui.netlify.app/experiments/docs/demos/ — the toolbar borderRadius: 0 is not being applied when openDemoSource is true (see how there is border radius on them). Similarly with the demoOptions condition.

    variants: [
      {
        props: ({ demoOptions }) => demoOptions.bg === 'inline',
        style: {
          marginTop: theme.spacing(1),
          borderTopWidth: 1,
        },
      },
      {
        props: ({ openDemoSource }) => openDemoSource,
        style: {
          borderRadius: 0,
        },
      },
    ],

This is a codemod bug, I will open a fix today.

Comment on lines +48 to +57
let Root = 'span';
if (shape === 'inline') {
Root = InlineShape;
}
if (shape === 'image') {
Root = ImageShape;
}
/* eslint-disable material-ui/no-hardcoded-labels, react/no-danger */
return (
<Root shape={shape === 'inline' ? 'inline' : adShape} className={className}>
<Root className={className}>
Copy link
Member

@oliviertassinari oliviertassinari Jun 23, 2024

Choose a reason for hiding this comment

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

This logic isn't equivalent. I have opened to fix this: #42735.

joserodolfofreitas pushed a commit to joserodolfofreitas/material-ui that referenced this pull request Jul 29, 2024
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
Development

Successfully merging this pull request may close these issues.

5 participants