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

[system][material-ui] Move useMediaQuery to system #39463

Merged
merged 13 commits into from
Feb 21, 2024
Merged

[system][material-ui] Move useMediaQuery to system #39463

merged 13 commits into from
Feb 21, 2024

Conversation

justintoman
Copy link
Contributor

@justintoman justintoman commented Oct 16, 2023

@mui-bot
Copy link

mui-bot commented Oct 16, 2023

Netlify deploy preview

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

@material-ui/system: parsed: +1.40% , gzip: +1.41%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against a184be6

@ZeeshanTamboli ZeeshanTamboli added package: system Specific to @mui/system package: joy-ui Specific to @mui/joy labels Oct 16, 2023
justintoman and others added 3 commits October 24, 2023 14:40
Co-authored-by: Siriwat K <siriwatkunaporn@gmail.com>
Signed-off-by: justintoman <justintoman@gmail.com>
@justintoman
Copy link
Contributor Author

I don't understand why the browser tests for joy began failing. This feels like an import issue, and it seems like it's only happening in bundled environments.

I've tried a number of things

  • Adding beforeEach/afterEach for mocking window.matchMedia from theme-scoping.test.tsx to various places, but it didn't fix everything, and I also feel like that's not a valid solution to this problem.
  • I tried messing with files that dealt with bundling or paths that mentioned useMediaQuery and changing it to come from system instead, but the tests still fail, window.matchMedia is still undefined.
    • scripts/sizeSnapshot/webpack.config.js
    • test/bundling/fixtures/next-webpack5/pages/next-webpack.fixture.js
    • test/bundling/scripts/fixtureTemplateValues.js
    • test/bundling/scripts/packages.js

I didn't commit any of that because it didn't solve anything.

Kinda stumped here 😅 would appreciate if someone could give me some help. 🙏

Sorry, thought this would be an easier lift. I'll keep trying but I don't feel like I have a clear idea of how to approach this.

@siriwatknp
Copy link
Member

I moved the test back to @mui/material but still got errors. It's weird.

@justintoman
Copy link
Contributor Author

Looks like it should be good to go, unless any of those changes I mentioned here for the bundling/test/webpack stuff need to go in.

@shadoath
Copy link

Watching to see when this is merged.

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 the PR!

@justintoman
Copy link
Contributor Author

This has been approved for a while... anything holding it up? Willing to update whatever is necessary!

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 15, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 20, 2024
@siriwatknp
Copy link
Member

This has been approved for a while... anything holding it up? Willing to update whatever is necessary!

I will merge after the CIs are green. Thanks for the PR and very sorry for the delay.

@siriwatknp
Copy link
Member

@michaldudak @Janpot Need your help, the CI error seems unrelated to the change. Can you guess what would be the root cause?

@michaldudak
Copy link
Member

Looks like we're low on memory during typechecking. Try reducing concurrency in the typescript:ci script

@Janpot
Copy link
Member

Janpot commented Feb 20, 2024

I think I ran into this as well last week and increased available memory for this script.
But decreasing concurrency will likely work as well

edit: @siriwatknp That PR got merged, you may want to try and rebase

@siriwatknp siriwatknp merged commit ba52e0d into mui:master Feb 21, 2024
19 checks passed
@cipherlogs
Copy link
Contributor

This needs to be added to the Joy-UI documentation.

With Material UI, we can import useMediaQuery using the normal path '@mui/material/useMediaQuery'. There is no need to update the Material UI docs. However, if using Joy UI, we have to install @mui/system, which is not mentioned in the documentation.

I hope this issue can be addressed.

#41402 It would also be better if Joy-UI moved away from useMediaQuery and added support for the breakpoints object using all props.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: joy-ui Specific to @mui/joy package: system Specific to @mui/system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants