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

useMediaQuery return wrong width #16460

Closed
2 tasks done
siriwatknp opened this issue Jul 3, 2019 · 12 comments · Fixed by #16611
Closed
2 tasks done

useMediaQuery return wrong width #16460

siriwatknp opened this issue Jul 3, 2019 · 12 comments · Fixed by #16611
Labels
good first issue Great for first contributions. Enable to learn the contribution process. hook: useMediaQuery

Comments

@siriwatknp
Copy link
Member

I found that useMediaQuery has some glitch when screen changes from big to small

  • This is not a v0.x issue.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior 🤔

useMediaQuery should return correct width when screen changes

Current Behavior 😯

When the screen change from large to small, there is one event that useMediaQuery return false

Steps to Reproduce 🕹

try this sandbox, I copy useWidth from the doc here

look at the console, stretch screen until you see "md" then narrow it. You will see one "xs".
https://codesandbox.io/s/material-ui-layout-v10-b4wil

image

Tech Version
Material-UI v4.1.3
React 16.8.6
Browser chrome
@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 3, 2019

@siriwatknp This is a limitation of the current implementation of the demo. You can find a version that is free from this glitch in #16127 but it relies on a private API.

Do you really need this useWidth logic? It's mostly documented for migration purposes, it's not the encourage default approach.

@siriwatknp
Copy link
Member Author

Actually, I only need a hook that return me the current width of the screen. I thought useWidth is something similar to withWidth. What is your suggestion?

@siriwatknp
Copy link
Member Author

The glitch is from useWidth logic or it is from useMediaQuery. I think the latter is the reason.

@oliviertassinari
Copy link
Member

The glitch comes from the useWidth demo usage. Can you work around the problem with a different usage strategy of useMediaQuery?

@siriwatknp
Copy link
Member Author

But I think the glitch is in useMediaQuery.
Take a look here. https://codesandbox.io/s/material-ui-layout-v10-b4wil
I change useWidth logic to something very simple. I keep resizing the screen but it still return null which means useMediaQuery return false for every breakpoint?

@oliviertassinari
Copy link
Member

@siriwatknp Nothing surprising from my perspective. I'm not aware of any guarantee in the order the different hooks will resolve. We might end up in an intermediary state where no media query matches.

@eps1lon
Copy link
Member

eps1lon commented Jul 5, 2019

Couldn't quite reproduce it though I did get the occasional null.

Wouldn't it make more sense to check if the screen size is above a certain breakpoint and the return the biggest one. For example:

function useWidth() {
  const theme = useTheme();
  const isXs = useMediaQuery(theme.breakpoints.up("xs"));
  const isSm = useMediaQuery(theme.breakpoints.up("sm"));
  const isMd = useMediaQuery(theme.breakpoints.up("md"));
  const isLg = useMediaQuery(theme.breakpoints.up("lg"));
  const isXl = useMediaQuery(theme.breakpoints.up("xl"));
  if (isXl) return "xl";
  if (isLg) return "lg";
  if (isMd) return "md";
  if (isSm) return "sm";
  if (isXs) return "xs";
  return null;
}

Solves the issue of holes between the intervals which removed the occasional null for me.

@siriwatknp Could you try that implementation and see if it solves your issue?

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 8, 2019

@eps1lon I confirm your proposal, it works better with :)

diff --git a/docs/src/pages/components/use-media-query/UseWidth.js b/docs/src/pages/components/use-media-query/UseWidth.js
index b727d4be2..4de165a40 100644
--- a/docs/src/pages/components/use-media-query/UseWidth.js
+++ b/docs/src/pages/components/use-media-query/UseWidth.js
@@ -14,7 +14,7 @@ function useWidth() {
   return (
     keys.reduce((output, key) => {
       // eslint-disable-next-line react-hooks/rules-of-hooks
-      const matches = useMediaQuery(theme.breakpoints.only(key));
+      const matches = useMediaQuery(theme.breakpoints.up(key));
       return !output && matches ? key : output;
     }, null) || 'xs'
   );

@siriwatknp
Copy link
Member Author

@eps1lon yes, that worked! thanks a lot.

@oliviertassinari
Copy link
Member

I'm reopening so we don't forget to update the demos. I will give it a go.

@siriwatknp
Copy link
Member Author

@oliviertassinari if you don't mind, I will update the demo and send a PR

@oliviertassinari
Copy link
Member

@siriwatknp Oh, yes, that would be awesome :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Great for first contributions. Enable to learn the contribution process. hook: useMediaQuery
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants