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] Fix transform not firing when theme provided #24010

Merged
merged 5 commits into from
Dec 14, 2020
Merged

[system] Fix transform not firing when theme provided #24010

merged 5 commits into from
Dec 14, 2020

Conversation

ZovcIfzm
Copy link
Contributor

@ZovcIfzm ZovcIfzm commented Dec 14, 2020 •
edited
Loading

Fixed the bug where transform was not fired if a theme was provided. Similarly, added tests to test this. Tests were provided from the changes in this thread (https://github.com/akomm/material-ui/blob/a4a394ff5420d29311ce1edef2bf20b51b507954/packages/material-ui-system/src/style.test.js)

And the change to style.js was provided in the issue discussion.

Fixes #23496

@ZovcIfzm
fixed/added tests for transform w. theme provided
65f341b 
fixed the bug where transform was not fired if a theme was provided. Similarly, added tests to test this. Tests were provided from the changes in this thread (https://github.com/akomm/material-ui/blob/a4a394ff5420d29311ce1edef2bf20b51b507954/packages/material-ui-system/src/style.test.js)

And the change to style.js was provided in the issue discussion.
@mui-pr-bot
Copy link

mui-pr-bot commented Dec 14, 2020 •
edited
Loading

Details of bundle changes

Generated by 🚫 dangerJS against 34295a2

@github-actions github-actions bot mentioned this pull request Dec 14, 2020
Closed
mnajdova
mnajdova reviewed Dec 14, 2020
View reviewed changes
Copy link
Member

@mnajdova mnajdova left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Changes look good, let's fix the tests and I believe it should be good to go :) Thanks for the contribution @ZovcIfzm

@oliviertassinari oliviertassinari changed the title [system] Fixed transform not firing when theme provided [system] Fix transform not firing when theme provided Dec 14, 2020
@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work package: system Specific to @mui/system labels Dec 14, 2020
@ZovcIfzm
Copy link
Contributor Author

Thank you! All my tests are passing except Vercel (which I think is a permission based check?). This will also be my very first open source contribution ever! I'm very excited. =)

mnajdova
mnajdova approved these changes Dec 14, 2020
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.

@ZovcIfzm this is a great first pull request! Congrats 🥳

The Vercel build is currently experimental on the repo, so don't worry about it! Everything looks good from my end :)

@ZovcIfzm
Copy link
Contributor Author

Completed! All tests except Vercel pass.

@mnajdova mnajdova merged commit fe14d4b into mui:next Dec 14, 2020
@ZovcIfzm ZovcIfzm deleted the transform-with-theme branch December 14, 2020 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliviertassinari oliviertassinari oliviertassinari left review comments

@mnajdova mnajdova mnajdova approved these changes

Assignees
No one assigned
Labels
bug 🐛 Something doesn't work package: system Specific to @mui/system
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[system] transform and themeKey do not work together when using style from material-ui/system
4 participants
@ZovcIfzm @mui-pr-bot @oliviertassinari @mnajdova