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.

Sign up for GitHub

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

Jump to bottom

[Link] Fix style overrides 5.6.0 regression #32182

Merged
merged 3 commits into from
Apr 11, 2022
Merged

[Link] Fix style overrides 5.6.0 regression #32182

merged 3 commits into from
Apr 11, 2022

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Apr 8, 2022 •
edited
Loading

Closes #32172
Closes #32183

Root cause

The prop color (default as primary) is passed to sx internally which has higher priority than style overrides. This PR check the instance color prop before passing it to sx.

<Link>...</Link> // ✅ style overrides (note that by overriding `textDecorationColor`, it removes the color calculation)
<Link color=...>...</Link> // goes to sx
<Link sx={{ color: ... }}> // goes to sx

Argos change is expected.

@siriwatknp siriwatknp changed the title [Link] Fix style overrides regression [Link] Fix style overrides 5.6.0 regression Apr 8, 2022
@siriwatknp siriwatknp added component: link This is the name of the generic UI component, not the React module! regression A bug, but worse labels Apr 8, 2022
@mui-bot
Copy link

mui-bot commented Apr 8, 2022 •
edited
Loading

Details of bundle changes

Generated by 🚫 dangerJS against 7190882

@siriwatknp siriwatknp requested a review from mnajdova April 8, 2022 02:38
siriwatknp
siriwatknp commented Apr 8, 2022
View reviewed changes
packages/mui-material/src/Link/Link.js
@@ -160,6 +160,7 @@ const Link = React.forwardRef(function Link(inProps, ref) {

return (
<LinkRoot
color={color}
Copy link
Member Author

Choose a reason for hiding this comment

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

bring back this line to send color to Typography for <Link /> basic use case.

siriwatknp
siriwatknp commented Apr 8, 2022
View reviewed changes
test/regressions/fixtures/Link/LinkColor.js
@@ -10,14 +10,14 @@ export default function DeterminateLinearProgress() {
MuiLink: {
styleOverrides: {
root: {
color: '#0068a9', // blue
color: '#fbca04', // orange
Copy link
Member Author

Choose a reason for hiding this comment

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

change to orange color to make the regression obvious

@tarmooo
Copy link

tarmooo commented Apr 11, 2022

Would be nice to have it merged :) Then could actually update to new version as right now all link colors are wrong and do not take account theme style overrides

@siriwatknp
Copy link
Member Author

Would be nice to have it merged :) Then could actually update to new version as right now all link colors are wrong and do not take account theme style overrides

For sure, I am waiting for the review. I think it can be merged today (usually, we release the package on Monday).

@siriwatknp siriwatknp merged commit e2cd553 into mui:master Apr 11, 2022
oliviertassinari
oliviertassinari reviewed Apr 11, 2022
View reviewed changes
packages/mui-material/src/Link/Link.js
@@ -149,7 +150,7 @@ const Link = React.forwardRef(function Link(inProps, ref) {
...props,
// It is too complex to support any types of `sx`.
// Need to find a better way to get rid of the color manipulation for `textDecorationColor`.
color: (typeof sx === 'function' ? sx(theme).color : sx?.color) || color,
color: (typeof sxColor === 'function' ? sxColor(theme) : sxColor) || color,
Copy link
Member

@oliviertassinari oliviertassinari Apr 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

IMHO

      <Link href="#unknown" color="#ff5252">
        #ff5252
      </Link>

and

      <Link href="#unknown" sx={{ color: '#ff5252' }}>
        #ff5252
      </Link>

should behave differently. I would expect the former (color prop) to set the color business logic of the component, e.g. the CSS color and CSS underline color, while the latter (sx prop) to only set the CSS color. I think that this would be correct:

Suggested change
color: (typeof sxColor === 'function' ? sxColor(theme) : sxColor) || color,
color,

Based on what I can see in https://www.argos-ci.com/mui/material-ui/builds/400. Anyway, nothing major, It's more that I feel we are overstepping a bit the scope responsibility of the sx prop.

Copy link
Member Author

@siriwatknp siriwatknp Apr 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Agree, I think it is too soon to add it. color in sx should only target CSS color property. I can open a PR to revert it before it is too late (it was introduced in the latest 5.6.1). @mnajdova What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Agree, the keys inside the sx prop should correspond to the CSS properties only

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliviertassinari oliviertassinari oliviertassinari left review comments

@michaldudak michaldudak michaldudak approved these changes

@mnajdova mnajdova Awaiting requested review from mnajdova

@hbjORbj hbjORbj Awaiting requested review from hbjORbj

Assignees
No one assigned
Labels
component: link This is the name of the generic UI component, not the React module! regression A bug, but worse
Projects
None yet
Milestone
No milestone
6 participants
@siriwatknp @mui-bot @tarmooo @oliviertassinari @mnajdova @michaldudak