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][system] Implementation of styled tag processor for linaria #38378

Merged
merged 12 commits into from
Aug 17, 2023

Conversation

brijeshb42
Copy link
Contributor

@brijeshb42 brijeshb42 commented Aug 8, 2023

This PR introduces two main packages

  1. zero-tag-processor - Package to contain all tag processors to be used by linaria for build time manipulation
  2. zero-runtime - The actual runtime implementation of styled that ends up in the user bundle.

What's done in the implementation -

  1. Processing css objects from styled calls, theme overrides, variants in styled call as well as theme object.
  2. CSS Variables for runtime styling based on component props. See.

What's left to implement (only in context of styled call) -

  • Optional processing of shorthands
  • Replacing usage of palette.primary.main (theme tokens) inside the dynamic functions with equivalent css variables at build time.
  • Restricting usage of theme object inside above functions
  • Generating theme classes with all the theme tokens

Moving some of the todos to independent PRs for easy review -

  • Copying over some of the logic from @mui/system to the package
  • Typescript typings for styled
  • Tagged template support. This version only support css objects.

There is also a test vite app to verify the changes in development. Note that this is not a workspace project yet as I need to figure out how to import items from ts file so that it can be used in vite.config.ts. So we need to manually run yarn install and then yarn dev to start the app.

@brijeshb42
Copy link
Contributor Author

@mnajdova @oliviertassinari One point to note in the ZeroSlider file is that some of the dynamic styles were made possible by using css variables in the css object definitions. So I think we should have first party support for declaring css variables. Something similar to Stitches as demonstrated here - https://github.com/brijeshb42/vite-example-no-stitches/blob/main/src/components/Slider/Slider.tsx#L183-L201

Comment on lines 193 to 212
/**
* Order of processing styles -
* 1. CSS directly declared in styled call
* 2. CSS declared in theme object's styledOverrides
* 3. Variants declared in styled call
* 3. Variants declared in theme object
*/
Copy link
Member

Choose a reason for hiding this comment

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

We will need to review this order. For e.g. now, whatever is defined in the theme would override the styles defined in the component (including "variants" styles). What I mean by this is:

const theme = createTheme({
  components: {
    MuiButton: { 
      styleOverrides: { // or variants with props {}
        root: { 
           background: '#00FF00',
        }
      }
    }
  }
});

Will override the button's background regardless of what color is used. We can come back to this later tough, I am just mentioning it for awareness.

Copy link
Member

Choose a reason for hiding this comment

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

One more question, how do we make sure that the component that is wrapped has it's styles defined previously? In cases like:

styled(Slider)

Copy link
Contributor Author

@brijeshb42 brijeshb42 Aug 9, 2023

Choose a reason for hiding this comment

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

Even if you define variants in base css, they get added after styleOverrides css.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One more question...

It's made sure by virtue of the bundler. If a component A is imported from A.ts like -

A.tsx

export const A = styled()...

and imported in B.tsx

import {} from './A';

const B = styled(A)(...)

CSS class ordering is in the order of the import. So first styled call in A gets processed and then in B

.class-A {}
.class-B {}

Copy link
Member

Choose a reason for hiding this comment

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

Even if you define variants in base css, they get added after styleOverrides css.

This makes sense to me, however is different than the current behavior, which may cause some issues. But again, let's revisit this later and focus on the other aspects.

@brijeshb42 brijeshb42 force-pushed the zero-cssjs branch 2 times, most recently from 4c74c5e to 3e49813 Compare August 9, 2023 17:19
@mui-bot
Copy link

mui-bot commented Aug 9, 2023

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 8c9a938

@brijeshb42 brijeshb42 force-pushed the zero-cssjs branch 7 times, most recently from f8e6abb to a6d2efa Compare August 10, 2023 11:54
@brijeshb42
Copy link
Contributor Author

Added 3 new commits in the PR to

  1. Replace usage of theme tokens in runtime functions with it's equivalent css variable.
  2. Generate theme css with all the possible tokens.

@oliviertassinari oliviertassinari changed the title [zero][system]: Implementation of styled tag processor for linaria [zero][system] Implementation of styled tag processor for linaria Aug 13, 2023
@brijeshb42 brijeshb42 force-pushed the zero-cssjs branch 2 times, most recently from 91ac318 to 23870e9 Compare August 14, 2023 07:36
@brijeshb42
Copy link
Contributor Author

@mnajdova Moved over some of the TODOs to the package README as it would be better reviewable individually. So for now, this PR is feature complete.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 15, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 16, 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.

two nits, implementation wise I think we can go with this for a first iteration. For me, the most important bit is to plan out the testing strategy for it.

apps/zero-runtime-vite-app/src/Slider/ZeroSlider.jsx Outdated Show resolved Hide resolved
apps/zero-runtime-vite-app/src/Slider/ZeroSlider.jsx Outdated Show resolved Hide resolved
object inside runtime css functions.

Also remove default export eslint rule.
@brijeshb42 brijeshb42 force-pushed the zero-cssjs branch 2 times, most recently from ad99766 to 9743885 Compare August 16, 2023 11:24
@brijeshb42
Copy link
Contributor Author

@mnajdova Builds are passing now. I'll merge once you approve.

@brijeshb42 brijeshb42 enabled auto-merge (squash) August 17, 2023 07:41
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.

Looks good for a first iteration. I would propose creating a page next for the lighthouse score performance test. For the next iteration, would be great to add next.js integration too.

@brijeshb42 brijeshb42 enabled auto-merge (squash) August 17, 2023 10:40
@brijeshb42 brijeshb42 enabled auto-merge (squash) August 17, 2023 10:40
@mnajdova mnajdova disabled auto-merge August 17, 2023 10:42
@mnajdova mnajdova enabled auto-merge (squash) August 17, 2023 10:42
@brijeshb42
Copy link
Contributor Author

There's one more PR for sx prop support. After that I'll create two more packages. One for a vite plugin and other for nextjs.

@brijeshb42 brijeshb42 merged commit d9a12c8 into mui:master Aug 17, 2023
18 checks passed
@brijeshb42 brijeshb42 deleted the zero-cssjs branch August 17, 2023 13:33
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
Projects
Status: Recently completed
Development

Successfully merging this pull request may close these issues.

5 participants