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 themeable component guide #37908

Merged
merged 18 commits into from
Aug 7, 2023

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Jul 11, 2023

Material UI guide: https://deploy-preview-37908--material-ui.netlify.app/material-ui/guides/themeable-component/
Joy UI guide: https://deploy-preview-37908--material-ui.netlify.app/joy-ui/guides/themeable-component/

All of the APIs are public but we've never published the docs. I think it's time to do it. Once the content is stable, I will add the same to Joy UI.

closes #32967

Further improvements in v6 (breaking changes)

The case transforming logic can be removed completely to make the API more intuitive (what you pass in is what you get). Existing usage on Material UI and Joy UI needs to be updated accordingly.

// as an example, Joy UI Button.tsx
export const ButtonRoot = styled('button', {
  name: 'JoyButton',
+  slot: 'root',
-  slot: 'Root',
-  overridesResolver: (props, styles) => styles.root,
})(…)

@siriwatknp siriwatknp added docs Improvements or additions to the documentation customization: theme Centered around the theming features labels Jul 11, 2023
@mui-bot
Copy link

mui-bot commented Jul 11, 2023

@siriwatknp
Copy link
Member Author

@mnajdova I think the styled can be simplified a bit.

  1. At this point, the overridesResolver is required to make the component themable but I think 99% of the custom component does not need custom overridesResolver (I think overridesResolver exists to make v4 -> v5 possible)

I think developers should provide just name and slot:

const StatSlot = styled('div', {
  name: 'MuiStat',
  slot: 'Root',
})()
  1. I think it will be better to use slot as a lower case or camel case. Basically whatever the slot value that you provide will be the key that the users will have to use.

Right now it's kinda confusing why I have to use Root in the slot but the users will use root in the theme.

I think both points won't be a breaking change.

cc @DiegoAndai

@siriwatknp siriwatknp marked this pull request as ready for review July 11, 2023 08:40
@siriwatknp
Copy link
Member Author

@samuelsycamore I put this in the how-to guide, not the customization. What do you think?

Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

Nice example! Easy to follow and understand 😊🎉

  1. Regarding overridesResolver, what do you propose? if nothing is provided, then the style override slot styles won't be added. Should we default to styleOverrides[slotName]?
  2. Regarding the slot casing, I agree we could use camelCase in this guide so it's the same as the key the users will use. Should we do that from now on in our own codebase?


Finally, you have to add the Stat component the theme types.

```ts
Copy link
Member

Choose a reason for hiding this comment

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

This example was confusing to me. I didn't understand what each override and change did. I think it would be better if each line is explained with a comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I will add a comment to it.

@siriwatknp
Copy link
Member Author

  • Regarding overridesResolver, what do you propose? if nothing is provided, then the style override slot styles won't be added. Should we default to styleOverrides[slotName]?

I have the same thought. It should default to styleOverrides[slotName]. I don't have a use case to customize it in Joy UI so I expect the others will be the same.

  • Regarding the slot casing, I agree we could use camelCase in this guide so it's the same as the key the users will use. Should we do that from now on in our own codebase?

We should but we have to update the implementation because some code is checking the upper first letter like Root. If @mnajdova agrees with this, I will update it in this PR (no breaking change).

@DiegoAndai
Copy link
Member

I agree with both changes proposed by @siriwatknp in the comment above 😊

@mnajdova
Copy link
Member

At this point, the overridesResolver is required to make the component themable but I think 99% of the custom component does not need custom overridesResolver (I think overridesResolver exists to make v4 -> v5 possible)
I think developers should provide just name and slot:

const StatSlot = styled('div', {
  name: 'MuiStat',
  slot: 'Root',
})(…)

We can make it optional 👍

I think it will be better to use slot as a lower case or camel case. Basically whatever the slot value that you provide will be the key that the users will have to use.
Right now it's kinda confusing why I have to use Root in the slot but the users will use root in the theme.

The slot is used for the display name - the name of the components, this is why initially it was uppercased. The overrides resolver is the thing that decides which key will be used in the style overrides. It could depend on the state and other factors, not just the slot name.

@omerman
Copy link

omerman commented Jul 18, 2023

This looks great! Im looking forward to work using these Apis 🙌

@@ -219,7 +233,7 @@ export default function createStyled(input = {}) {
if (process.env.NODE_ENV !== 'production') {
let displayName;
if (componentName) {
displayName = `${componentName}${componentSlot || ''}`;
displayName = `${componentName}${capitalize(componentSlot || '')}`;
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 based on your comment, I think the logic can be the other way around.

I believe that using slotName directly is easier to understand for developers. The capitalize I added is to preserve the previous behavior. In fact, it can be removed because it is for debugging and it won't be used in production.

We could remove it in the next major release?

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 we can keep using capitalize it in the displayName

@siriwatknp
Copy link
Member Author

Nice example! Easy to follow and understand 😊🎉

  1. Regarding overridesResolver, what do you propose? if nothing is provided, then the style override slot styles won't be added. Should we default to styleOverrides[slotName]?
  2. Regarding the slot casing, I agree we could use camelCase in this guide so it's the same as the key the users will use. Should we do that from now on in our own codebase?

I have updated:

  1. added default overridesResolver, so it will use styleOverrides[slotName] as you suggested.
  2. Now, the slot casing can be anything, we should let developers own this part (from the code, it won't have an impact on the theming logic, meaning whatever slot name they define the styleOverrides will have to be that name)

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.

The implementation looks good. Let's clarify in the TODO which version we are talking about, so that we can search easily. The copy-writing can be reviewed by @samuelsycamore

@siriwatknp siriwatknp changed the title [docs] Add custom component guide [docs] Add themable component guide Jul 20, 2023
@siriwatknp siriwatknp changed the title [docs] Add themable component guide [docs] Add themeable component guide Jul 20, 2023
@samuelsycamore
Copy link
Member

I'm on it! Thanks for being patient! 🫡

@siriwatknp siriwatknp added the package: system Specific to @mui/system label Jul 21, 2023
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 29, 2023
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.

I'm so sorry it took me so long to get back to this @siriwatknp !! 😭 I assume that most of my comments here apply to the Material UI doc as well.

siriwatknp and others added 4 commits August 3, 2023 08:38
Co-authored-by: Sam Sycamore <71297412+samuelsycamore@users.noreply.github.com>
Signed-off-by: Siriwat K <siriwatkunaporn@gmail.com>
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 3, 2023
@siriwatknp
Copy link
Member Author

@samuelsycamore all fixed, including Material UI guide.

@mnajdova mnajdova merged commit b97f61f into mui:master Aug 7, 2023
21 checks passed
richbustos pushed a commit that referenced this pull request Aug 7, 2023
@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 10, 2024

I'm late to the party, but this change makes a lot of sense, great to see steps made here 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customization: theme Centered around the theming features docs Improvements or additions to the documentation package: system Specific to @mui/system
Projects
Status: Recently completed
Development

Successfully merging this pull request may close these issues.

Give the possibility to create new Components using the Material theming system for unique behavior
7 participants