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.

Sign up for GitHub

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

Jump to bottom

[system] Add empty interfaces to fix issue with typescript module augmentation #43873

Merged
merged 5 commits into from
Oct 3, 2024
Merged

[system] Add empty interfaces to fix issue with typescript module augmentation #43873

merged 5 commits into from
Oct 3, 2024

Conversation

yonatan0
Copy link
Contributor

@yonatan0 yonatan0 commented Sep 24, 2024 •
edited by mnajdova
Loading

Fixes #43855

@mui-bot
Copy link

mui-bot commented Sep 24, 2024 •
edited
Loading

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 1b2fe45

@Janpot Janpot added package: system Specific to @mui/system enhancement This is not a bug, nor a new feature labels Sep 24, 2024
@Janpot Janpot changed the title [mui/system] add empty interfaces to fix issue with typescript module augmentation [system] Add empty interfaces to fix issue with typescript module augmentation Sep 24, 2024
Janpot
Janpot reviewed Sep 24, 2024
View reviewed changes
packages/mui-system/src/createTheme/createTheme.d.ts Outdated
mixins?: unknown;
typography?: unknown;
mixins?: Mixins;
typography?: Typography;
zIndex?: unknown;
Copy link
Member

Choose a reason for hiding this comment

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

The only unknown left is zIndex. Is there a reason not to type it the same way as ThemeOptions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my bad, fixed 🙏

@yonatan0
Copy link
Contributor Author

yonatan0 commented Sep 25, 2024 •
edited
Loading

  1. Changed the zIndex type from Record<string, number> to a more generic interface: The original type was causing conflicts with @mui/material's createTheme function.
  2. Renamed zIndex to ZIndex: to comply with the linting rules that enforce PascalCase naming conventions.

@yonatan0
Copy link
Contributor Author

hey @Janpot,
i tried to figure out why the argos ci step failed, but with no success, maybe you have some insights for me on how to fix this?

Janpot
Janpot approved these changes Sep 25, 2024
View reviewed changes
Copy link
Member

@Janpot Janpot left a comment

Choose a reason for hiding this comment

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

This seems good to me

maybe you have some insights for me on how to fix this?

This screenshot is flaky, we haven't fully figured out why yet. You can safely ignore

@Janpot Janpot requested a review from mnajdova September 25, 2024 14:51
mnajdova
mnajdova approved these changes Oct 3, 2024
View reviewed changes
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.

It's a great first conutrion on MUI 👌 People will appreciate the fix! :)

@mnajdova mnajdova merged commit e430fcc into mui:master Oct 3, 2024
19 checks passed
@yonatan0 yonatan0 deleted the create-theme-interfaces branch October 3, 2024 12:05
@mnajdova mnajdova mentioned this pull request Oct 14, 2024
Closed
This was referenced Oct 31, 2024
Merged
Merged
@Souviksamanta34 Souviksamanta34 mentioned this pull request Oct 31, 2024
Open
@bansikah22 bansikah22 mentioned this pull request Oct 31, 2024
Merged
This was referenced Oct 31, 2024
Open
Open
This was referenced Nov 1, 2024
Open
Open
@josetutis josetutis mentioned this pull request Nov 2, 2024
Open
@Hoang-Nguyen-Huy Hoang-Nguyen-Huy mentioned this pull request Nov 2, 2024
Merged
@Hoang-Nguyen-Huy Hoang-Nguyen-Huy mentioned this pull request Nov 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Janpot Janpot Janpot approved these changes

@mnajdova mnajdova mnajdova approved these changes

Assignees
No one assigned
Labels
enhancement This is not a bug, nor a new feature package: system Specific to @mui/system
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[mui/system] theme - typescript module augmentation
4 participants
@yonatan0 @mui-bot @Janpot @mnajdova