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

[material-ui][SvgIcon] Convert to support CSS extraction #41779

Merged
merged 8 commits into from
Apr 25, 2024

Conversation

aarongarciah
Copy link
Member

@aarongarciah aarongarciah commented Apr 5, 2024

Part of #41273

No visual changes expected.

before after
localhost_3000_material-ui_icons localhost_3000_material-ui_icons (2)
localhost_3000_material-ui_icons (1) localhost_3000_material-ui_icons (3)

Original description I tried to follow these steps to generate the demos:
  1. pnpm install
  2. node scripts/pigmentcss-render-mui-demos.mjs icons (this differs from other component docs pages that are prefixed with react-)
  3. pnpm build
  4. cd apps/pigment-css-next-app && pnpm dev

But the pnpm build command is failing in the TwoToneIcons.js|tsx demo:

Cannot read properties of null (reading 'palette')
image

@aarongarciah aarongarciah added component: SvgIcon The React component. package: material-ui Specific to @mui/material package: pigment-css Specific to @pigment-css/* v6.x labels Apr 5, 2024
@siriwatknp
Copy link
Member

siriwatknp commented Apr 5, 2024

From the developer's code:

import * as React from 'react';
import { useTheme } from '@mui/material/styles';
import Icon from '@mui/material/Icon';

const useIsDarkMode = () => {
  const theme = useTheme();
  return theme.palette.mode === 'dark';
};

export default function TwoToneIcons() {
  const isDarkMode = useIsDarkMode();

  return (
    <Icon
      sx={{ ...(isDarkMode && { filter: 'invert(1)' }) }}
      baseClassName="material-icons-two-tone"
    >
      add_circle
    </Icon>
  );
}

The error occur during evaluation phase, here is the eval code:

"use strict";

Object.defineProperty(exports, "__esModule", {
  value: true
});
exports.__wywPreval = void 0;
var _styles = require("@mui/material/styles");
var _Icon = _interopRequireDefault(require("@mui/material/Icon"));
function _interopRequireDefault(obj) { return obj && obj.__esModule ? obj : { default: obj }; }
const useIsDarkMode = () => {
  const theme = (0, _styles.useTheme)();
  return theme.palette.mode === "dark";
};
let isDarkMode = useIsDarkMode();
const _exp = /*#__PURE__*/() => ({
  ...(isDarkMode && {
    filter: "invert(1)"
  })
});
const _exp2 = /*#__PURE__*/() => _Icon.default;
const __wywPreval = exports.__wywPreval = {
  _exp: _exp,
  _exp2: _exp2
};

I think it's related to sx preprocessor, Pigment CSS should never use those variables from the call, instead create those variables at the root scope so that it always pass the evaluation:

"use strict";

Object.defineProperty(exports, "__esModule", {
  value: true
});
exports.__wywPreval = void 0;

let isDarkMode;
const _exp = /*#__PURE__*/() => ({
  ...(isDarkMode && {
    filter: "invert(1)"
  })
});
const _exp2 = /*#__PURE__*/() => _Icon.default;
const __wywPreval = exports.__wywPreval = {
  _exp: _exp,
  _exp2: _exp2
};

cc @brijeshb42 I'd consider this as a bug, what do you think?

@brijeshb42
Copy link
Contributor

@siriwatknp True. This is a bug. Right now, the pending thing to handle in sx transform is conditional runtime spread. theme spread is handled but runtime spread is not.

@mui-bot
Copy link

mui-bot commented Apr 8, 2024

Netlify deploy preview

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

packages/material-ui/material-ui.production.min.js: parsed: +0.08% , gzip: +0.05%
StepIcon: parsed: +0.70% , gzip: +0.34%
Avatar: parsed: +0.66% , gzip: +0.28%
TextField: parsed: +0.26% , gzip: +0.19%
SpeedDialIcon: parsed: +0.66% , gzip: +0.37%
NativeSelect: parsed: +0.49% , gzip: +0.29%
Breadcrumbs: parsed: +0.46% , gzip: +0.26%
TabList: parsed: +0.41% , gzip: +0.20%
AvatarGroup: parsed: +0.59% , gzip: +0.25%
SvgIcon: parsed: +0.67% , gzip: +0.29%
TableSortLabel: parsed: +0.49% , gzip: +0.21%
StepLabel: parsed: +0.59% , gzip: +0.25%
StepButton: parsed: +0.45% , gzip: +0.18%
@material-ui/core: parsed: +0.07% , gzip: +0.03%
TabScrollButton: parsed: +0.48% , gzip: +0.21%
Rating: parsed: +0.56% , gzip: +0.19%
PaginationItem: parsed: +0.46% , gzip: +0.12%
Pagination: parsed: +0.44% , gzip: +0.11%
Radio: parsed: +0.47% , gzip: +0.11%
Autocomplete: parsed: +0.26% , gzip: +0.06%
and 4 more changes

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 8a3fa51

@aarongarciah aarongarciah marked this pull request as ready for review April 8, 2024 15:30
@aarongarciah
Copy link
Member Author

@siriwatknp CI is now passing after applying Brijesh suggestion.

baseClassName="material-icons-two-tone"
style={isDarkMode ? { filter: 'invert(1)' } : undefined}
Copy link
Member

@siriwatknp siriwatknp Apr 9, 2024

Choose a reason for hiding this comment

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

I don't think we should change this. We should create a fix from Pigment CSS first and then apply it to this PR. It's okay to have this PR open and label it as on hold because it is blocked by another issue.

If you don't mind @aarongarciah, let's wait for a fix from @brijeshb42 and then apply to this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, let's wait.

Copy link
Member Author

Choose a reason for hiding this comment

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

I reverted this change.

Copy link
Contributor

@brijeshb42 brijeshb42 Apr 9, 2024

Choose a reason for hiding this comment

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

TBH, the fix will take some time. It's not just about the sx transform here but also about the support for transformed sx prop to be passed to the Icon that Marija is working on.
Even right now, one particular transform is supported, that is, sx={isDarkMode ? {} : undefined}. This would work as far as transformation is concerned. But we'll still need to support the sx prop in underlying component.
If there is an alternate, we should do that and not wait for a proper fix.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note: moving from sx to style changes the specificity.

Copy link
Contributor

Choose a reason for hiding this comment

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

True. But it's a component in our docs, not a generic component or library component. So it should be fine since it doesn't affect end-users.

@aarongarciah aarongarciah added the on hold There is a blocker, we need to wait label Apr 9, 2024
.filter(([, value]) => value.main || value.action || value.disabled)
.map(([color]) => ({
props: { color },
style: { color: theme.palette?.[color]?.main },
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
style: { color: theme.palette?.[color]?.main },
style: { color: (theme.vars ?? theme).palette?.[color]?.main },

x the other usages.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated (except function calls like theme.typography?.pxToRem() or theme.transitions?.create() which don't exist in the vars object).

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.

👍 I realize that this PR should not be blocked. I removed the demo that caused the error from this PR which will be fixed by mui/pigment-css#35.

@siriwatknp siriwatknp enabled auto-merge (squash) April 25, 2024 09:30
@siriwatknp siriwatknp merged commit c9bef2c into mui:next Apr 25, 2024
19 checks passed
@ZeeshanTamboli ZeeshanTamboli removed the on hold There is a blocker, we need to wait label Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: SvgIcon The React component. package: material-ui Specific to @mui/material package: pigment-css Specific to @pigment-css/* v6.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants