-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
[material-ui][SvgIcon] Convert to support CSS extraction #41779
Conversation
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 "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? |
@siriwatknp True. This is a bug. Right now, the pending thing to handle in |
Netlify deploy previewhttps://deploy-preview-41779--material-ui.netlify.app/ packages/material-ui/material-ui.production.min.js: parsed: +0.08% , gzip: +0.05% Bundle size reportDetails of bundle changes (Toolpad) |
@siriwatknp CI is now passing after applying Brijesh suggestion. |
baseClassName="material-icons-two-tone" | ||
style={isDarkMode ? { filter: 'invert(1)' } : undefined} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, let's wait.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted this change.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
.filter(([, value]) => value.main || value.action || value.disabled) | ||
.map(([color]) => ({ | ||
props: { color }, | ||
style: { color: theme.palette?.[color]?.main }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: { color: theme.palette?.[color]?.main }, | |
style: { color: (theme.vars ?? theme).palette?.[color]?.main }, |
x the other usages.
There was a problem hiding this comment.
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).
54e92e9
to
4e66d18
Compare
…arciah/pigment-mui-svgicon
There was a problem hiding this 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.
Part of #41273
No visual changes expected.
Original description
I tried to follow these steps to generate the demos:pnpm install
node scripts/pigmentcss-render-mui-demos.mjs icons
(this differs from other component docs pages that are prefixed withreact-
)pnpm build
cd apps/pigment-css-next-app && pnpm dev
But the
pnpm build
command is failing in theTwoToneIcons.js|tsx
demo: