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

[test] Do not enforce the presence of ownerState.className in describeConformance #44479

Merged

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Nov 20, 2024

In @mui/x-date-pickers and @mui/x-date-pickers-pro, we are updating all our ownerState (both the one used in slotProps and the one used in styled() or styleOverrides to be a small object shared across all the slots of the library.

The idea behind this change is that, from a user point of view, it is unclear which part of this UI resolves those slots since the component are very large. So the props used for the ownerState is hardly predictable and does not contain the same information from one slot to another, often missing key information of the current state of the picker.

I encountered a small problem because describeConformance uses ownerState.className which is no longer present in our components for some tests. Do you see a problem hardcoding a value instead?

@mui-bot
Copy link

mui-bot commented Nov 20, 2024

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 1d88653

@michaldudak
Copy link
Member

This changes the meaning of this test entirely. The purpose of it is to test if the ownerState in callback has the props passed to the component. Since your components work differently, IMO skipping this test would make more sense.

@flaviendelangle
Copy link
Member Author

We should update the name of the test then, IMHO it does not describe what you are saying.
It would be great if we could have 2 tests then, one that tests the props and what that only test the firing of the callback.
That way we can skip the one that tests the props without loosing what is interesting to us (the firing of the callback).

@michaldudak
Copy link
Member

Yup, 2 different tests are a good option as well.

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.

This looks good to me.

@flaviendelangle
Copy link
Member Author

flaviendelangle commented Nov 21, 2024

I added the 2nd test 👍
And the CI is passing

Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

LGTM. 👍

A good compromise to cater for both the @mui/material as well as the transitional approach on mui-x@v8 👌

@flaviendelangle flaviendelangle merged commit 6894dfa into mui:master Nov 22, 2024
19 checks passed
@flaviendelangle flaviendelangle deleted the describeConformance-ownerState branch November 22, 2024 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants