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-ui] Introduce color inversion utilities #38916

Merged
merged 91 commits into from
Oct 4, 2023

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Sep 11, 2023

Sandbox: https://codesandbox.io/s/joy-ui-cra-ts-forked-h8prkz?file=/src/App.tsx

closes #38912

For detailed motivation, read #38912.

The utilities let developers apply the color inversion on any component.

Breaking Changes

  • Color inversion context has been removed. To apply color inversion on any parent component, use an independent function named applySolidInversion or applySoftInversion to the sx prop or styled.

    + import { applySolidInversion } from '@mui/joy/styles';
    
    <Box
     sx={[
       { bgcolor: 'black', },
       applySolidInversion('neutral'),
     ]}
    >
  • Child components that have explicit color prop will use the color inversion variables by default. To opt out for a specific component, set data-skip-inverted-colors attribute to the component.

    <Card invertedColors variant="solid" color="primary">
      <div>
         <Stack>
            <Button variant="soft" color="warning"
                    data-skip-inverted-colors // to prevent the button from being inverted
            > 

Todo

  • Remove context subscription from all components
  • Update docs

@mui-bot
Copy link

mui-bot commented Sep 11, 2023

Netlify deploy preview

@mui/joy: parsed: +0.60% , gzip: -0.20% 😍

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 394f4e0

@siriwatknp siriwatknp requested a review from mnajdova September 11, 2023 10:06
@siriwatknp
Copy link
Member Author

@mnajdova This PR is the changes I mentioned to you last week. It's the draft to prove that the new implementation works better and is easier to customize. Please have a look and if it looks good to you, I will apply the changes to all components with more tests.

preview: https://deploy-preview-38916--material-ui.netlify.app/experiments/joy/color-inversion/

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.

This PR tackles much more than what the title indicates. It's not just about removing the context, it's changing the API too.

What do you think about doing the following:

  • keep the API (the invertedColors prop) for the components that we believe it should be supported in
  • export a applyColorInversion(variant, color) util for custom components (or one-off Joy UI components that we don't support it by default
  • remove the context usage.

* @param color a supported theme color palette
* @returns (theme: ThemeFragment) => Record<DefaultColorPalette, CSSObject>
*/
export const applySolidInversion = (color: ColorPaletteProp) => (theme: ThemeFragment) => {
Copy link
Member

Choose a reason for hiding this comment

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

Do we plan to expose 4 different methods? Can't we have one where we would accept the variant & the color?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we plan to expose 4 different methods? Can't we have one where we would accept the variant & the color?

We need 2 functions so far since only soft and solid come with a background. I think a separate function sounds better in the long term.

@siriwatknp
Copy link
Member Author

siriwatknp commented Sep 15, 2023

This PR tackles much more than what the title indicates. It's not just about removing the context, it's changing the API too.

What do you think about doing the following:

  • keep the API (the invertedColors prop) for the components that we believe it should be supported in
  • export a applyColorInversion(variant, color) util for custom components (or one-off Joy UI components that we don't support it by default
  • remove the context usage.

Sounds good to me. Changed the PR title to "Introduce color inversion utilities" and preserved the current API (invertedColors prop). I will create another PR to remove the color inversion context.

@siriwatknp siriwatknp changed the title [joy-ui] Remove color inversion context [joy-ui] Introduce color inversion utilities Sep 15, 2023
@siriwatknp siriwatknp added enhancement This is not a bug, nor a new feature new feature New feature or request and removed breaking change enhancement This is not a bug, nor a new feature labels Sep 18, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 24, 2023
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 27, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 29, 2023
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.

So much cleaner 🤩 I love that we staged the work into removing context only. I wonder if we even need to drop the feature at all, I think we can experiment with this for a while.

sx={[
(theme) => ({}),
// @ts-expect-error no `unknown` color from theme palette
applySoftInversion('unknown'),
Copy link
Member

Choose a reason for hiding this comment

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

Does it work with custom colors too if developers add them in the theme?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, they can add more color by augmenting the ColorPalettePropOverrides from the theme.

// explicit color. Color inversion has no effect.
<Chip variant="soft" color="primary">…</Chip>
```
:::info
Copy link
Member

Choose a reason for hiding this comment

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

Not a deal breaker for this PR, but we have too many info callouts on this page. Every second paragraph feels like a callout. I would rather convert them to text, as otherwise they don't serve the purpose to "call out".

@siriwatknp siriwatknp merged commit 73a7cde into mui:master Oct 4, 2023
9 checks passed
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Oct 9, 2023
@rogier-mars-mck
Copy link

With the new approach, how do I configure the settings for a particular variant (soft, solid) and color? Previously this could be done via the CssVarsThemeOptions['colorInversion'] property of the theme.

@siriwatknp
Copy link
Member Author

siriwatknp commented Nov 3, 2023

With the new approach, how do I configure the settings for a particular variant (soft, solid) and color? Previously this could be done via the CssVarsThemeOptions['colorInversion'] property of the theme.

The easiest and fastest way is to use sx prop:

<Card
  variant="solid"
  color="primary"
  invertedColors
  sx={theme => ({
    "& :not([data-skip-inverted-colors], [data-skip-inverted-colors] *)" {
      [theme.getColorSchemeSelector('light' | 'dark')]: {
        '--variant-plainColor': 
        
      }
    }
  })}
>

You just need to use the same selectors as it is internally used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change new feature New feature or request package: joy-ui Specific to @mui/joy
Projects
Status: Recently completed
Development

Successfully merging this pull request may close these issues.

[joy-ui] Remove context from color inversion
5 participants