-
-
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] Add channel colors if possible #35178
Conversation
@material-ui/core: parsed: +0.26% , gzip: +0.16% |
if (!obj[key] && defaultValue) { | ||
obj[key] = defaultValue; | ||
} |
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.
only apply the key if defaultValue exists.
// before
setColor(object, 'a', undefined); // { a: undefined }
// after
setColor(object, 'a', undefined); // {}
@mnajdova Please review again. In summary, instead of throwing errors, the warning is displayed for specific tokens that are used for channel color generation. |
setColor( | ||
palette.Alert, | ||
'infoColor', | ||
silent(() => darken(palette.info.light, 0.6)), |
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.
Can we extract this in a separate function? Instead of invoking silent(() => darken())
we could just invoke darken()
, which will behind the scene do this. Same for the other color manipulators. We could even export these functions from the color manipulators directly by using some prefix, like safeDarken
or something similar. My worry is that if we add new color tokens we may forget to use the silent
function.
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.
Something like this?
// colorManipulator.js
export const colorChannel = ...;
export const safeColorChannel = (color, warning) => {
try {
return colorChannel(color);
} catch (error) {
if (warning && process.env.NODE_ENV !== 'production') {
console.warn(warning);
}
return undefined;
}
}
// experimental_extendTheme.js
setColor(
palette[color],
'activeChannel',
colorChannelSafe(colors.active, 'MUI: The value of "palette.action.active" should be one of these formats: #nnn, #nnnnnn, rgb(), rgba(), hsl(), hsla(), color().'),
);
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.
@mnajdova In case you missed the notification.
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.
Exactly yes, sorry for not responding sooner.
private_safeColorChannel as safeColorChannel, | ||
private_safeAlpha as safeAlpha, | ||
private_safeDarken as safeDarken, | ||
private_safeLighten as safeLighten, | ||
private_safeEmphasize as safeEmphasize, |
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.
@mnajdova I mark them as private_
to align with our intention.
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.
Looks good
close #35159
Problem
https://codesandbox.io/s/determined-fermat-mikfcs?file=/src/App.tsx
The
extendTheme
creates channel colors if it seespalette.*.main
, so it breaks if I do this with custom palette:This is definitely confusing for developers and there is no workaround unless they change from
primary
to another name.Solution
Display warning instead of throwing errors.
For dependent tokens
These tokens are used to calculate channel colors, so the warning is displayed if it cannot parse the value:
For independent tokens
These tokens are not used by others, so they can be replaced with any CSS color, e.g. gradient or refer to another CSS variable.
Custom palettes
Like in #35159, the gradient is created with the same structure of MUI primary palette but that's not the intention of the developers, so the errors are silent:
https://codesandbox.io/s/awesome-chihiro-uo3ddh?file=/src/App.tsx