-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
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] Scope the tests to just Material UI components #35219
Conversation
This reverts commit ee3091f.
…slots-slotprops
|
test/utils/describeConformance.js
Outdated
}); | ||
} else { | ||
// eslint-disable-next-line react/prop-types | ||
const CustomComponent = React.forwardRef(({ className }, ref) => ( |
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.
Below are the same tests from #34873
@michaldudak Could you take a quick look at this? The change might be hard to review but it is all about moving the tests to Material UI scope so that I can add similar tests for Joy UI. The idea is to have a smooth transition for Material UI tests in v6, v7. |
I agree with the idea completely. Does this PR change |
test/utils/describeConformance.tsx
Outdated
|
||
if (!components) { | ||
// if `components` are not defined, do not run tests that depend on them | ||
filteredTests = filteredTests.filter((testKey) => !['componentsPropsProp'].includes(testKey)); |
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.
What about componentsProp
?
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.
Those tests are in function testMaterialUIComponentsPropsProp()
.
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 guess we will need another big refactoring but that can be done in a separate PR.
test/utils/describeConformance.tsx
Outdated
@@ -41,6 +41,7 @@ export interface InputConformanceOptions { | |||
mount: (node: React.ReactNode) => ReactWrapper, | |||
) => (node: React.ReactNode) => ReactWrapper; | |||
slots?: Record<string, SlotTestingOptions>; | |||
components?: Record<string, SlotTestingOptions>; |
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.
leave a comment explaining the current usage of components
and slots
?
e.g.,
slots
are for Material UI v6 and Joy UI
components
are for Material UI v6; Expected to be removed from v7
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.
Good point.
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.
Left a few comments.
Signed-off-by: Siriwat K <siriwatkunaporn@gmail.com>
What's the difference between Material UI v5 and Joy in terms of the |
For For
With the above conditions, mixing both packages in the same test suite would create a lot of if-else which is not good for maintainability and transition for Material UI. |
The idea for the slots/slotProps is to be as similar between products as possible, even today.
That's not true. Material UI components use camelCase as well: https://github.com/mui/material-ui/blob/master/packages/mui-material/src/Alert/Alert.d.ts#L114-L116
This isn't something that needs v6 to be implemented. |
My bad, I meant Material UI
Okay, I got your point. Will update the PR but won't include implementation to support callback, is this align with you? |
I'd say we should leave the |
This reverts commit 3a6b30a.
…/use-components
Updated! |
'componentsProp', | ||
'slotsProp', | ||
'reactTestRenderer', | ||
'slotPropsCallback', |
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.
Would you mind adding a comment to explain why this is skipped (for all instances)? // not supported yet
should be enough.
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.
Sure, thanks for asking.
}); | ||
} | ||
|
||
function testSlotPropsCallback(element: React.ReactElement, getOptions: () => ConformanceOptions) { |
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.
👍
The current conformance test suite makes it hard to test
slots
prop on Joy UI components #34997 because it is used in Material UI tests.This PR moves the
slots
tocomponents
so that it reflects only Material UI components. This will make the transition toslots
prop easier for Material UI because, in v6, theslots
prop will behave the same as Joy UI.Changes
slots
key tocomponents
key to test Material UI components (in [Joy] Addslots
/slotProps
props to the typing of all components and applyuseSlot
to all components #34997, Joy UI will useslots
key). The test logics remain the same.The future
#34997
Joy UI will run the
slots
prop tests which are separate from the current Material UI tests, allow them to coexist in the samedescribeConformance
.// mui-joy/src/Alert/Alert.test.tsx describeConformance({ ... + slots: { root: { ... } } })
v6
Material UI: add
slots
key to conformance which runs the same tests as Joy UI.components
tests remain the same.describeConformance({ ... + slots: { root: { ... } } })
v7
Material UI: remove
components
tests because it is removed from the prop.describeConformance({ ... - components: { root: { ... } } })