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

[Switch] Fix DOM warning when type isn't checkbox or radio #36170

Merged
merged 3 commits into from Feb 16, 2023

Conversation

dani-mp
Copy link
Contributor

@dani-mp dani-mp commented Feb 13, 2023

Fixes #36111.

@dani-mp dani-mp changed the title Patch 1 Fix DOM warning when using Switch Feb 13, 2023
@mui-bot
Copy link

mui-bot commented Feb 13, 2023

Netlify deploy preview

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

Bundle size report

Details of bundle changes

Generated by 🚫 dangerJS against ffde56d

@siriwatknp siriwatknp changed the title Fix DOM warning when using Switch [Switch] Fix DOM warning when it is labelled Feb 14, 2023
@siriwatknp siriwatknp added bug 🐛 Something doesn't work component: switch This is the name of the generic UI component, not the React module! PR: needs test The pull request can't be merged package: material-ui Specific to @mui/material labels Feb 14, 2023
@siriwatknp
Copy link
Member

@dani-mp Hey, thanks a lot. The fix looks good to me but could you add a test to verify?

@dani-mp
Copy link
Contributor Author

dani-mp commented Feb 14, 2023

@siriwatknp the fix is pretty small and IMO it should have been avoided using a linter rule or something like that. Not sure you want tests for every prop you pass to a component in your library.

In any case, I'm curious, how would you test this, given that it's a DOM warning? Can you point me to a place where you do this already?

@michaldudak
Copy link
Member

Not sure you want tests for every prop you pass to a component in your library.

We generally want tests for every fixed issue.

In any case, I'm curious, how would you test this, given that it's a DOM warning?

Our tests are set up to fail if there's any error on the console during execution. Simply rendering a switch with a type other than checkbox or radio will be enough.

@siriwatknp
Copy link
Member

@dani-mp This should work:

diff --git a/packages/mui-material/src/Switch/Switch.test.js b/packages/mui-material/src/Switch/Switch.test.js
index 1c9a99278c..df8724a5c2 100644
--- a/packages/mui-material/src/Switch/Switch.test.js
+++ b/packages/mui-material/src/Switch/Switch.test.js
@@ -137,4 +137,8 @@ describe('<Switch />', () => {
       });
     });
   });
+
+  it('should not show warnings when custom `type` is provided', () => {
+    expect(() => render(<Switch type="submit" />)).not.toErrorDev();
+  });
 });

@michaldudak michaldudak changed the title [Switch] Fix DOM warning when it is labelled [Switch] Fix DOM warning when type isn't checkbox or radio Feb 15, 2023
@dani-mp
Copy link
Contributor Author

dani-mp commented Feb 15, 2023

Done! ✅

Copy link
Member

@michaldudak michaldudak left a comment

Choose a reason for hiding this comment

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

Thanks a lot! It's a great first PR in Material UI!

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.

👍 Thanks for your time and welcome to MUI contributors. Looking forward to the next PR.

@ZeeshanTamboli ZeeshanTamboli removed the PR: needs test The pull request can't be merged label Feb 16, 2023
@michaldudak michaldudak merged commit ef3fb7e into mui:master Feb 16, 2023
@dani-mp dani-mp deleted the patch-1 branch February 16, 2023 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: switch This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch • Warning: Received false for a non-boolean attribute id
5 participants