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

[zero] Add README with installation and basic usage #40761

Merged
merged 3 commits into from
Feb 1, 2024

Conversation

brijeshb42
Copy link
Contributor

@brijeshb42 brijeshb42 added the package: pigment-css Specific to @pigment-css/* label Jan 23, 2024
@mui-bot
Copy link

mui-bot commented Jan 23, 2024

Netlify deploy preview

https://deploy-preview-40761--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against c2ce6bd

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.

Very exciting stuff!

@@ -0,0 +1,238 @@
# zero-runtime

A zero-runtime CSS-in-JS library that extracts the colocated css to it's own css files at build-time.
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
A zero-runtime CSS-in-JS library that extracts the colocated css to it's own css files at build-time.
A zero-runtime CSS-in-JS library that extracts colocated CSS to separate CSS files at build time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this sound like that each css definition will have its own css file?

Copy link
Member

Choose a reason for hiding this comment

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

It wasn't very clear to me exactly what was intended by the original sentence. Is there another way to phrase this? Maybe it requires more details?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically, it depends on the route. So for ex, for next.js, each route might have its own css file. So all styled calls' CSS will be part of that route's css file.
Maybe just - that extracts the colocated CSS to the output CSS files

packages/zero-runtime/README.md Outdated Show resolved Hide resolved
packages/zero-runtime/README.md Outdated Show resolved Hide resolved
packages/zero-runtime/README.md Outdated Show resolved Hide resolved
packages/zero-runtime/README.md Outdated Show resolved Hide resolved
packages/zero-runtime/README.md Outdated Show resolved Hide resolved
packages/zero-runtime/README.md Outdated Show resolved Hide resolved
packages/zero-runtime/README.md Outdated Show resolved Hide resolved
packages/zero-runtime/README.md Outdated Show resolved Hide resolved
packages/zero-runtime/README.md Outdated Show resolved Hide resolved
const { experimental_extendTheme: extendTheme } = require('@mui/material/styles');
const theme = extendTheme();
Copy link
Member

Choose a reason for hiding this comment

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

What if we don't mention @mui/material/styles and use a plain theme object?

Suggested change
const { experimental_extendTheme: extendTheme } = require('@mui/material/styles');
const theme = extendTheme();
const theme = {
colorSchemes: {
light: {
palette: {
brand: '...',
}
},
dark: {
palette: {
brand: '...',
}
}
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We right now depend on the extendTheme implementation to then call generateCssVars in the zero-runtime util's generateCss. If we have to support plain objects, we can either implement the same in zero-runtime directly or we can move the theme implementation to @mui/theming package that will only have all theme specific logic.

Copy link
Member

@siriwatknp siriwatknp Jan 31, 2024

Choose a reason for hiding this comment

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

Got it. I think we need to revisit the theming for zero-runtime. In my opinion, zero-runtime should work without theme config.

If developer tries to use theme, we throw the error/warning and show them where to add the theme.

Next, the theme object can be a plain object that they can control. Meaning, if I provide:

const theme = {
  typography: {
    h1: {},}
}

I should be able to use it from the theme callback

Then the CSS variables, we should consider as an opt-in feature. Meaning, developers have to follow our structure and pass in generateCssVars to the theme.

I can take care of this task if you agree with the above model.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally, yes. It should work even with plain js objects. This was the case actually previously before we agreed on relying on making extendTheme the default. We can still provide css variable token object even if they are not using extendTheme. It would be possible if we extract out the css vars related utilities to either @mui/system or independent package.

@brijeshb42 brijeshb42 force-pushed the zero-readme branch 2 times, most recently from b0bf4b3 to 535c448 Compare January 31, 2024 07:51
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.

👍

@brijeshb42 brijeshb42 merged commit b7b84a6 into mui:master Feb 1, 2024
19 checks passed
mostafa-rio pushed a commit to mostafa-rio/material-ui that referenced this pull request Feb 3, 2024
@oliviertassinari oliviertassinari added the package: system Specific to @mui/system label Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: pigment-css Specific to @pigment-css/* package: system Specific to @mui/system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants