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

[system] Reexport Pigment CSS from index file #43218

Merged
merged 8 commits into from
Aug 7, 2024

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Aug 7, 2024

Issue

The changes fixe theme augmentation issue with the latest beta:

Change

Remove the @mui/material-pigment-css/theme and reexport from the index directly. Using theme subfolder adds unnecessary complexity and require user to import type {} from @mui/material-pigment-css/theme first.

The change require users to set up TypeScript as below (documented in the migration guide):

declare module "@mui/material-pigment-css" {
  interface ThemeArgs {
    theme: Theme
  }
}

@mui-bot
Copy link

mui-bot commented Aug 7, 2024

@siriwatknp
Copy link
Member Author

siriwatknp commented Aug 7, 2024

This requires import {} from '@mui/material-pigment-css/theme' at least once to be able to augment correctly. For example,

import {} from '@mui/material-pigment-css/theme';
import { Theme } from '@mui/material/styles';

declare module "@mui/material-pigment-css/theme" {
  interface ThemeArgs {
    theme: Theme
  }
}

I wonder if it makes more sense to augment the theme from @mui/material-pigment-css instead of theme subpath. cc @brijeshb42

@siriwatknp siriwatknp changed the title [material-pigment-css] Move theme.ts into subpath for module augmentation [material-pigment-css] Reexport Pigment CSS from index file Aug 7, 2024
@siriwatknp
Copy link
Member Author

@brijeshb42 Verified that the change is working as expected.

Comment on lines 79 to 81
const pigmentConfig = {
transformLibraries: ['@mui/material'],
};
Copy link
Member

Choose a reason for hiding this comment

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

Some of these seem to have leaked from #43217?

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 reverted the changes on the migration-to-v6 which are not related to this PR. It looks good to me

@mnajdova mnajdova merged commit 8636fbe into mui:next Aug 7, 2024
22 checks passed
@siriwatknp
Copy link
Member Author

I reverted the changes on the migration-to-v6 which are not related to this PR. It looks good to me

Thanks @mnajdova

@@ -200,6 +200,35 @@ pnpm dev

Open the browser and navigate to the localhost URL, you should see the app running with Pigment CSS.

### Typescript
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
### Typescript
### TypeScript

Fixing this in #43751

@oliviertassinari oliviertassinari added package: pigment-css Specific to @pigment-css/* package: system Specific to @mui/system and removed package: material-ui Specific to @mui/material package: pigment-css Specific to @pigment-css/* labels Sep 14, 2024
@oliviertassinari oliviertassinari changed the title [material-pigment-css] Reexport Pigment CSS from index file [system] Reexport Pigment CSS from index file Sep 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: system Specific to @mui/system typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants