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

[core] Implement useTheme processor and runtime theme #105

Merged
merged 4 commits into from
Jun 4, 2024

Conversation

brijeshb42
Copy link
Contributor

@brijeshb42 brijeshb42 commented May 27, 2024

Here's how the replacement works

Before transformation:

import { useTheme } from '../zero-styled';

function Component() {
  const theme = useTheme();
  // ...rest
}

After transformation:

import _theme from '@pigment-css/react/theme';

function Component() {
  const theme = _theme;
  // ...rest
}

Fixes: mui/material-ui#42260

@brijeshb42 brijeshb42 added the new feature New feature or request label May 27, 2024
@brijeshb42 brijeshb42 changed the title Core/implement use theme [core] Implement useTheme processor and runtime theme May 27, 2024
@brijeshb42 brijeshb42 closed this May 27, 2024
@brijeshb42 brijeshb42 reopened this May 27, 2024
@brijeshb42 brijeshb42 force-pushed the core/implement-use-theme branch 2 times, most recently from a7558aa to 38fcae8 Compare May 28, 2024 11:10
Comment on lines 10 to 16
const breakpointsImport = hasBreakpoints
? "import createBreakpoints from '@mui/system/createTheme/createBreakpoints';\n"
: '';
const transitionsImport = hasTransitions
? `import createTransitions from '${process.env.PACKAGE_NAME}/createTransitions';\n`
: '';
const source = `${breakpointsImport}${transitionsImport}
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 this should come from Material UI. Pigment should just call theme.toRuntime() and let Material UI decide what to add to the runtime.

In Material UI, we can export an adapter function that inserts .toRuntime() to the theme.

// next.config.js
import { createPigmentConfig, extendTheme } from '@mui/material/styles';

const config = createPigmentConfig(extendTheme());

module.exports = withPigment(config);

Copy link
Member

@siriwatknp siriwatknp May 30, 2024

Choose a reason for hiding this comment

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

If you agree with my proposal, I can create a PR to add the createPigmentCssConfig().

Copy link
Member

Choose a reason for hiding this comment

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

Agree with having this in Material UI side, and by default Pigment CSS can only add the CSS vars. What do you think?

Copy link
Member

@siriwatknp siriwatknp May 31, 2024

Choose a reason for hiding this comment

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

Yes, I think Pigment should call specific methods rather than owning the logic inside.

To let Pigment generate CSS variables from the theme:

create a method generateStyleSheets to theme. At build time, Pigment will create CSS file from the result of the method.

To create a runtime theme:

create a method toRuntime to theme. At build time, Pigment will use that result and export as JS (the result has to be a string)

To use the runtime theme:

import theme from '@pigment-css/react/theme';

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

siriwatknp commented Jun 1, 2024

Here is my proposal, Material UI export a stringifyTheme function mui/material-ui#42476 for zero-runtime libraries.

From Pigment side, I see 2 choices:

  1. Pigment calls a method from the theme, e.g. theme.toRuntime():
 // next.config.js
const { extendTheme, stringifyTheme } = require('@mui/material/styles');

const theme = extendTheme();

theme.toRuntime = () => stringifyTheme(theme);

module.exports = withPigment({
  theme,
})
  1. Pigment read from the config
 // next.config.js
const { extendTheme, stringifyTheme } = require('@mui/material/styles');

const theme = extendTheme();

module.exports = withPigment({
  theme,
  runtimeTheme: stringifyTheme(theme),
})

I like this approach because regardless of the choice, it's not related to Material UI. In the future, stringifyTheme could be moved to Material UI plugin for Pigment.

@brijeshb42
Copy link
Contributor Author

I like the first approach better, ie, theme.toRuntime. This way, users won't have to care about anything extra. We can make toRuntime method part of the createTheme/extendTheme from material-ui source code.
In future, this will anyways won't be part of the Pigment v1 release since the whole thing is very specific to Material UI. So we can move this over to the Material specific package.

I am naming the method to toRuntimeSource to be more explicit.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 3, 2024
@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
Copy link
Member

@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.

Preapproved, please check my suggestions before merging.

to replace the useTheme function calls with an import from Pigment
package
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 4, 2024
@brijeshb42 brijeshb42 merged commit ab5aabb into mui:master Jun 4, 2024
11 of 12 checks passed
@brijeshb42 brijeshb42 deleted the core/implement-use-theme branch June 4, 2024 04:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[material-ui][pigment-css] useTheme integration
3 participants