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

[joy] Refine the default theme #36843

Merged
merged 333 commits into from
Jul 29, 2023
Merged

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Apr 11, 2023

closes #35788
closes #37663
closes #36782
closes #37663
closes #37967
closes #38165

Migration docs: https://deploy-preview-36843--material-ui.netlify.app/joy-ui/migration/migrating-default-theme/
New Tabs design: https://deploy-preview-36843--material-ui.netlify.app/joy-ui/react-tabs/
Migration tested: https://codesandbox.io/s/joy-cra-ts-forked-mtdp6g?file=/src/App.tsx

Try it out

We really appreciate it if you could try the new changes in your existing Joy UI project.

  1. Replace the @mui/joy version in your package.json with this:

    - "@mui/joy": "5.0.0-alpha.x"
    + "@mui/joy": "https://pkg.csb.dev/mui/material-ui/commit/2c04ead4/@mui/joy"
  2. run yarn install (you might have to run it twice because the first time might see an error).

  3. Follow the migration docs

Review recommendation

Better to go through the migration docs because it is the source of all breaking changes.

Changes

Tokens removed

  • palette.info
  • letterSpacing

Typography

h1
h2
h3
h4
- h5
- h6
- body1
- body2
- body3
- body4
- body5
+ title-lg
+ title-md
+ title-sm
+ body-lg
+ body-md
+ body-sm
+ body-xs

The composition of title-* and body-* is improved. When using them together with an icon, there is no need to add extra margin at all. For example:

// Before
<SvgIcon sx={{ mt: '2px' }}>
<div>
  <Typography fontWeight="lg" mb={0.25}>Title</Typography>
  <Typography level="body2">Description</Typography>
</div>

// After
<SvgIcon>
<div>
  <Typography level="title-md">Title</Typography>
  <Typography level="body-sm">Description</Typography>
</div>
Before After
image image
image image

Templates

Before After
image image
image image
image image
image image

@siriwatknp siriwatknp added on hold There is a blocker, we need to wait package: joy-ui Specific to @mui/joy design: joy This is about Joy Design, please involve a visual or UX designer in the process labels Apr 11, 2023
@siriwatknp siriwatknp added this to the Joy UI stable release milestone Apr 11, 2023
Copy link
Member Author

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

@danilo-leal I started by removing some font-size, font-weight and levels that are not likely to be used to keep the theme as lean as possible.

For font-size, I think if it is lower than 12px it is very hard to read so I don't think < xs should be in the default theme.

For h5 and h6, I follow tailwindcss typography:

But now we have. Please don't use h5 or h6 in your content, Medium only supports two heading levels for a reason, you animals. I honestly considered using a before pseudo-element to scream at you if you use an h5 or h6.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 1, 2023
sm: '0.875rem', // 14px
md: '1rem', //16px
lg: '1.25rem', // 20px
xl:'1.5rem', // 24px
Copy link
Contributor

Choose a reason for hiding this comment

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

I've reduced the amount of typography, but didn't delete the remain font sizes, should we do it?

md: '12px',
lg: '16px',
xl: '20px',
xs: '2px',
Copy link
Contributor

Choose a reason for hiding this comment

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

I've reduced a bit the radius, but it's not set in stone, what do you think?

@zanivan
Copy link
Contributor

zanivan commented Jun 1, 2023

Sorry @siriwatknp, I got carried away and ended up committing all the changes at once 😅
I've let some comments on the changes, but it'd be nice to review everything and reassure that I didn't do anything wrong.

Besides the comments I left in the file, I'd like to ask a few more things:

  1. How/where is defined the placeholder texts?
  2. How should we remove the "info" palette?
  3. Could we have more semantic names for texts sizes? I think it'd improve the user experience, providing names like "Title-xl", "Title-md", "body-sm", and so on. What do you think?

@siriwatknp
Copy link
Member Author

How/where is defined the placeholder texts?

We don't have a theme token that is specific to the placeholder text. They can be configured through component variables, for example, the Input:

image

How should we remove the "info" palette?

I can help with that.

Could we have more semantic names for texts sizes? I think it'd improve the user experience, providing names like "Title-xl", "Title-md", "body-sm", and so on. What do you think?

Do you mean in theme.typography? The benefit I see is that they are more explicit than body1, 2, 3 (who knows if 1 is bigger or smaller than 2). However, we should do the POC of the typography set with demo components and templates too validate that they are useful and intuitive.

@zanivan
Copy link
Contributor

zanivan commented Jun 2, 2023

We don't have a theme token that is specific to the placeholder text. They can be configured through component variables, for example, the Input:

Would it be possible to have a token for it? I think opacity could be a bit tricky to contrast when dealing with so many variables.

Do you mean in theme.typography? The benefit I see is that they are more explicit than body1, 2, 3 (who knows if 1 is bigger or smaller than 2). However, we should do the POC of the typography set with demo components and templates too validate that they are useful and intuitive.

I'm not sure if it'd be in theme.typography, but the idea is to have something like <Typography level="body-lg">Hello, World!</Typography>, instead of <Typography level="body1">Hello, World!</Typography>. I reckon this would really help in moments where the user needs a smaller text, and could just add a sm instead of remembering which body to use. But yeah, definitely should try on POCs before.

@siriwatknp
Copy link
Member Author

Would it be possible to have a token for it? I think opacity could be a bit tricky to contrast when dealing with so many variables.

What do you think about adding the token to palette.text.placeholder?

@zanivan
Copy link
Contributor

zanivan commented Jun 5, 2023

What do you think about adding the token to palette.text.placeholder?

I was testing the contrast here and probably we would have to have one for each variant (plain, solid,soft and outlined). What do you think we could do then?

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

siriwatknp commented Jun 9, 2023

Opportunities for improvement:

  • Line height seems wrong (using rem) 👉 /experiments/joy/prose/
  • Disabled state 👉 /experiments/joy/global-variants/ with neutral palette
  • neutral can be darker 👉 /experiments/joy/global-variants/ (will try different tokens for some components)
  • more typography levels
  • remove letter spacing scale? Yes.
  • color token for icon, e.g. palette.text.icon. We should have tokens for color with states.
  • add shadow-opacity

@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Jun 13, 2023
@zanivan
Copy link
Contributor

zanivan commented Jun 15, 2023

I was looking at the components at /experiments/joy/global-variants/ and the only thing that still needs major adjustments is the primary warning card. Isn't it following the tokens set on extendedTheme?

@zanivan
Copy link
Contributor

zanivan commented Jun 16, 2023

@siriwatknp I've updated the typography according to what we discussed on the Figma file.

Also, I was making some tests at /experiments/joy/global-variants/ and I think we could bump up a bit the opacity of placeholders. I tried 0.7, and I think it's a sweet spot between being recognized as a placeholder, but still being able to be differentiated from the filled label.

@zanivan
Copy link
Contributor

zanivan commented Jun 19, 2023

@siriwatknp I tweaked the shadows, surfaces, some icon colors, and the disabled button. Also, I updated the email template to remove the custom styles to make it closer to the default as possible.

@siriwatknp
Copy link
Member Author

@zanivan I adjust the global variant tokens a bit (disabled and some neutral palette tokens). I think the before version is too light and too close to the disabled state, so I increase the contrast a little bit more and also make the disabled state lighter. Let me know what you think!

Before:
image

After:
image

@siriwatknp
Copy link
Member Author

siriwatknp commented Jul 27, 2023

@siriwatknp When do You plan to merge it? I could really use to this changes.

@td5999 I plan to merge it this week and it will be released next week with v5.0.0-beta.0.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 27, 2023
@siriwatknp siriwatknp added customization: theme Centered around the theming features and removed on hold There is a blocker, we need to wait labels Jul 27, 2023
@sernstberger
Copy link
Contributor

These changes are great. It's exciting that Joy is moving to beta!

@Studio384
Copy link
Contributor

@Studio384 @td5999 Would you mind checking the "Try it out" in the PR description? Would love you to see if there is anything missing before we merge this PR.

I generally like the changes, I do prefer the old grey scale tho, so definitly going to restore that in the implementations we use (or at least the blue shine in it). :) Anyways great work!

@siriwatknp
Copy link
Member Author

@Studio384 @td5999 Would you mind checking the "Try it out" in the PR description? Would love you to see if there is anything missing before we merge this PR.

I generally like the changes, I do prefer the old grey scale tho, so definitly going to restore that in the implementations we use (or at least the blue shine in it). :) Anyways great work!

Thanks for the feedback! in that case you can take a look at this migration doc on how to use the previous neutral and primary colors.

@mwskwong
Copy link
Contributor

mwskwong commented Jul 28, 2023

I tried it for a while. And I notice some things that we might want to improve.

  1. Default icon color - Should --joy-palette-text-icon be darker instead? It looks a bit out of place.
    image
    image

  2. BG color in light color scheme - body and level1 shares the same color code, unlike dark color scheme, not sure if that's intentional.

  3. The default body text color becomes --joy-palette-text-secondary instead of --joy-palette-text-primary.

@siriwatknp
Copy link
Member Author

siriwatknp commented Jul 29, 2023

@mwskwong Thanks for the feedback!

Default icon color - Should --joy-palette-text-icon be darker instead? It looks a bit out of place.

I feel like the icon color should be lighter than the text to be complementary. That's why I choose neutral.400 as a default icon color.

Some inspiration:

No worry if you want to change it, you can override it via the theme:

extendTheme({
  colorSchemes: {
    light: {
      palette: {
        text: {
          icon: 'var(--joy-palette-neutral-600)',
        }
      }
    },
    dark: {},
  }
})

BG color in light color scheme - body and level1 shares the same color code, unlike dark color scheme, not sure if that's intentional.

I think @zanivan has fixed it in the latest commit.

The default body text color becomes --joy-palette-text-secondary instead of --joy-palette-text-primary.

Yes, the previous version does not have much contrast between texts. I feel like the new version looks a lot better.

Anyway, you can override the theme level as you like:

extendTheme({
  typography: {
    'body-md': {
      color: 'var(--joy-palette-text-primary)',
    },}
})

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 5, 2023

I didn't have the time to dive into this PR, instead I put together all the opportunities that I could spot in the latest release at https://www.notion.so/mui-org/Olivier-design-review-on-Joy-Design-3ada9a7bcfa44b9fab1fe5032dfb33bb and public link.

Overall, I love that we are allocating time to shape the look & feel, I think it's a high ROI effort 👍

@clarkmcc
Copy link

clarkmcc commented Sep 19, 2023

I'm not sure if this was the change that caused it, but some recent design changes seemed to break the design of my existing project, and event some of the examples, for example the solid card design doesn't look quite right.

image

@danilo-leal
Copy link
Contributor

@clarkmcc Hey! Feel free to open an issue in case we can help with any issues due to recent Joy UI updates ⎯ anyhow, it's super cool you're using it; appreciate you giving it a shot!

In this case, although it definitely doesn't look nice, it's not technically broken. That's due to Joy UI's global variants feature, which allows all components to have the same variant set, and their styles to be defined in one place only. It's conceptually a sweet feature and, overall, works fine. But, there are many cases, such as this Card one, where certain variants simply don't make sense (or even look broken, as you pointed out). The tentative solution we're flirting with is "turning off" non-sensical variants for specific components, which still allows us to keep the benefits of globally defined variant styles. There are also other options in the table and it's generally something we want to sort through before the stable version ⎯ so, keep an eye out!

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 design: joy This is about Joy Design, please involve a visual or UX designer in the process package: joy-ui Specific to @mui/joy
Projects
Status: Recently completed
10 participants