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

[system][createStyled] Intercept ownerState coming from props and ownerState #42358

Merged
merged 3 commits into from
May 28, 2024

Conversation

DiegoAndai
Copy link
Member

@DiegoAndai DiegoAndai commented May 23, 2024

From #42345 (comment)

This resolves the styled symptoms of #42184 by not allowing ownerState values coming from props or ownerState to override the component's ownerState.

It does not close #42184, as nested ownerState values are still being propagated.

@DiegoAndai DiegoAndai added the package: system Specific to @mui/system label May 23, 2024
@DiegoAndai DiegoAndai requested a review from mnajdova May 23, 2024 12:27
@DiegoAndai DiegoAndai self-assigned this May 23, 2024
@DiegoAndai DiegoAndai changed the title [system][createStyled] Intercept props and ownerState ownerState key [system][createStyled] Intercept ownerState coming from props and ownerState May 23, 2024
@mui-bot
Copy link

mui-bot commented May 23, 2024

Netlify deploy preview

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

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 30ec290

@DiegoAndai
Copy link
Member Author

See Argos changes for the fixes

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.

There are few more lines where we handle this, e.g.

typeof callableStyle === 'function' ? callableStyle({ ownerState, ...props }) : callableStyle;
and
processStyleArg(resolvedStyle, { ownerState, ...props }),

Can we create a const at the top of the function and used that in all places?

@DiegoAndai
Copy link
Member Author

@mnajdova I refactored by creating a const and added a test.

For the occurrences you point out, there's no risk of overriding. The function signature is

function processStyleArg(callableStyle, { ownerState, ...props })

So props would never have an ownerState key. Because of this lines 51 and 55 won't show this issue, and we can leave them as is.

@DiegoAndai DiegoAndai requested a review from mnajdova May 27, 2024 20:34
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.

Test, logic and argos look good to me 👌 We should port this logic to Pigment CSS too.

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
None yet
Development

Successfully merging this pull request may close these issues.

[material-ui] Avoid ownerState silent propagation
3 participants