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

[Popper] Add missing styleOverrides Popper type in theme #39154

Merged
merged 4 commits into from
Oct 9, 2023

Conversation

axelbostrom
Copy link
Contributor

@axelbostrom axelbostrom commented Sep 25, 2023

@mui-bot
Copy link

mui-bot commented Sep 25, 2023

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against ee83ff6

@axelbostrom axelbostrom reopened this Sep 25, 2023
@zannager zannager added the component: Popper The React component. See <Popup> for the latest version. label Sep 25, 2023
@zannager zannager requested a review from mnajdova September 25, 2023 15:10
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

@axelbostrom Thank you for resolving this issue! Importing PopperClassKey from MUI Base is indeed the correct approach. Could you also include a TypeScript test to ensure its proper functionality in the createTheme.spec.ts file at this location? This test will help confirm that everything is working as expected.

packages/mui-material/src/styles/components.d.ts Outdated Show resolved Hide resolved
@ZeeshanTamboli ZeeshanTamboli added bug 🐛 Something doesn't work typescript package: material-ui Specific to @mui/material labels Sep 27, 2023
@ZeeshanTamboli ZeeshanTamboli changed the title [Popper][material-ui] styleOverride fix for Popper [Popper][material-ui] Fix styleOverrides type in theme Sep 27, 2023
Co-authored-by: Zeeshan Tamboli <[email protected]>
Signed-off-by: Axel Boström <[email protected]>
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

@axelbostrom Could you act on this comment as well to add a test?

@axelbostrom
Copy link
Contributor Author

axelbostrom commented Sep 29, 2023

@ZeeshanTamboli I am trying yes, but have some minor issues writing the test. Looking at it now and will see if I can solve it, otherwise maybe you could assist me?

Edit: I am a little confused to what I should do in the .spec.ts file. When adding things here, nothing seems to happen. Should I not add the tests to the test.js file? If so, then I am done with the tests.

@mj12albert
Copy link
Member

mj12albert commented Oct 5, 2023

I am a little confused to what I should do in the .spec.ts file

diff --git a/packages/mui-material/src/styles/createTheme.spec.ts b/packages/mui-material/src/styles/createTheme.spec.ts
index f86a12a414..897077f2b1 100644
--- a/packages/mui-material/src/styles/createTheme.spec.ts
+++ b/packages/mui-material/src/styles/createTheme.spec.ts
@@ -66,6 +66,13 @@ const theme = createTheme();
 {
   createTheme({
     components: {
+      MuiPopper: {
+        styleOverrides: {
+          label: {
+            color: 'black',
+          },
+        },
+      },
       MuiFormControlLabel: {
         styleOverrides: {
           root: {

@axelbostrom Add another key like this, and you can run the type tests locally from the repo root with yarn typescript or yarn workspace @mui/material typescript

The corresponding job in the Ci is ci/circleci: test_types

@ZeeshanTamboli
Copy link
Member

I am a little confused to what I should do in the .spec.ts file. When adding things here, nothing seems to happen

@axelbostrom Adding tests to the .spec.ts file will verify the correctness of types and detect any type errors if there are unexpected changes in the types you've defined for Popper in component styleOverrides.

Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

@axelbostrom I added the pending TypeScript test. Thank you for your contribution! Looking forward for more.

@ZeeshanTamboli ZeeshanTamboli changed the title [Popper][material-ui] Fix styleOverrides type in theme [Popper] Add missing styleOverrides Popper type in theme Oct 9, 2023
@ZeeshanTamboli ZeeshanTamboli merged commit 81b101d into mui:master Oct 9, 2023
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: Popper The React component. See <Popup> for the latest version. package: material-ui Specific to @mui/material typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Popper][material-ui] styleOverrides type is missing for MuiPopper in theme
5 participants