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] Improve the createBox types #35532

Merged
merged 10 commits into from
Jan 12, 2023
Merged

[system] Improve the createBox types #35532

merged 10 commits into from
Jan 12, 2023

Conversation

mnajdova
Copy link
Member

Fixes #35506

@mnajdova mnajdova added typescript package: system Specific to @mui/system labels Dec 19, 2022
@mui-bot
Copy link

mui-bot commented Dec 19, 2022 •
edited
Loading

Messages
📖 Netlify deploy preview: https://deploy-preview-35532--material-ui.netlify.app/

No bundle size changes

Generated by 🚫 dangerJS against d360a98

michaldudak
michaldudak reviewed Jan 3, 2023
View reviewed changes
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.

It would be great to have a test that verifies the solution. I tried to recreate the original problem using packages from this PR, and it still fails.

packages/mui-material/src/Box/Box.d.ts Outdated Show resolved Hide resolved
packages/mui-system/src/createBox.d.ts Outdated Show resolved Hide resolved
@mnajdova
Copy link
Member Author

mnajdova commented Jan 3, 2023

It would be great to have a test that verifies the solution. I tried to recreate the original problem using packages from this PR, and it still fails.

I will resolve the build issues and create tests tomorrow.

@mnajdova mnajdova mentioned this pull request Jan 4, 2023
Closed
2 tasks
@mnajdova mnajdova marked this pull request as ready for review January 4, 2023 12:32
@mnajdova
Copy link
Member Author

mnajdova commented Jan 4, 2023

@michaldudak this is ready for final review. I've added tests in the Box.spec.tsx, and here is a sandbox with the reproduction from the issue: https://codesandbox.io/s/damp-water-uo6nsc?file=/src/App.tsx

@mnajdova
Update packages/mui-system/src/createBox.d.ts
d360a98 
Signed-off-by: Marija Najdova <mnajdova@gmail.com>
@mnajdova mnajdova requested review from michaldudak and a team January 10, 2023 13:09
michaldudak
michaldudak approved these changes Jan 11, 2023
View reviewed changes
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.

This should work :) 👍

@mnajdova mnajdova merged commit 5d55067 into mui:master Jan 12, 2023
@pH-7
Copy link

pH-7 commented Jan 20, 2023 •
edited
Loading

BTW @mnajdova @michaldudak, not sure if this should be mentioned on the release changelog (or perhaps, it should have been tagged as a major semantic version) as these changes would be a breaking change for some projects as it would give some errors with older TS compilers which doesn't support such a syntax (e.g. with tsc 4.2.4 version).

Screen Shot 2023-01-20 at 1 17 16 pm

@mnajdova mnajdova mentioned this pull request Jan 20, 2023
Merged
@Wire1lovet
Copy link

Null

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaldudak michaldudak michaldudak approved these changes

Assignees
No one assigned
Labels
package: system Specific to @mui/system typescript
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Box from @mui/material/Box and return from createBox return different types
5 participants
@mnajdova @mui-bot @pH-7 @Wire1lovet @michaldudak