Skip to content
This repository has been archived by the owner on Jun 22, 2020. It is now read-only.

If the screen width is greater or equal than 1920 the screen prop is 'xs' when should be 'xl' #3

Closed
donatoaguirre24 opened this issue Jul 10, 2019 · 10 comments

Comments

@donatoaguirre24
Copy link

Hi, first of all, congrats for the excellent job, you are killing it.

I noticed an issue when the screen width is greater or equal than 1920, at that point the screen prop is set as 'xs' when should be 'xl'.

I take a look at the code and I think the issue was caused by the function getScreen in src/hooks/useWidth.js

You are initializing screen at 'xs' and only changing that value on:

if (width < breakpoints.values[key]) {
  screen = key;
  return true;
} 

The maximum value that breakpoints.values[key] takes is 1920 so if width is greater or equal than that, the screen variable stays as 'xs'. I could be easily fixed adding an:

else if (width >= 1920) { 
  screen = 'xl'; 
  return true;
}

Of course, should exist a more elegant solution, I threw the first that came to my mind.

It would be great if you can fix it, but if you're too busy now, tell me and I'll make a PR 👍

@siriwatknp
Copy link
Owner

My mistake! I will fix this issue today and also this one mui/material-ui#16460. It will be shipped in v1.2.4

siriwatknp added a commit that referenced this issue Jul 10, 2019
@siriwatknp
Copy link
Owner

@donatoaguirre24 please pull dev and try again

@donatoaguirre24
Copy link
Author

Thanks for the quick response. I tried the version 1.2.4 but looks like there is something that is breaking the hooks rules (change in the order of Hooks called by Nav), I'm getting the following error:

image

By the way, you forgot to remove the console.log(screen) in src/components/Root.js line 17.

Tell me if you need more info or captures of the console errors.

@siriwatknp
Copy link
Owner

@donatoaguirre24 do you use create-react-app? I don't have this issue in both of my projects in v1.2.4. Can you provide give me your repo?

@donatoaguirre24
Copy link
Author

Yes, I'm using create-react-app with react-scripts 3.0.1. I can't provide you the repo because it's a production project of the company and I'm not allowed to share it.

I'm nearly crazy, I already check al the code and I can't figure out what's causing the issue. I 'm sure that it's the Nav component because when I remove it all works fine but I don't understand why.

React is complaining about useStyles(props) in the nav component but I don't know why this may change the order of the hooks calls. The exact error is the following:

image

image

@donatoaguirre24
Copy link
Author

I put a sample in Codesandbox and I'm getting the same error, you can check it here: https://codesandbox.io/s/material-ui-layout-v12-j0goe

@siriwatknp
Copy link
Owner

I’ll help u until u get it, dont worry!

@siriwatknp
Copy link
Owner

I think something is wrong in your project because I create new project with create-react-app and follow my doc. Everything works just fine. May be you should delete node_modules and then reinstall again.

@donatoaguirre24
Copy link
Author

I finally figured out how to fix it 🙌🏻. I had already tried to delete the node_modules but this didn't help me. The error was caused by the config object particularly the "clipped" field, I don't know why but when I removed the following all started to work just fine:

  clipped: {
    xs: false,
    sm: false,
    md: false
  },

My last request is if you can remove the console.log(screen) in src/components/Root.js line 17 because it's no suitable to have it in production.

Thank u very much for taking time to help me, and for this wonderful layout builder, continue with the great work 👍

@siriwatknp
Copy link
Owner

siriwatknp commented Jul 13, 2019

@donatoaguirre24 I removed it in v1.2.4 and I have no idea why clipped props cause you this issue. Anyway, thank you so much.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants