nuxt-viewport icon indicating copy to clipboard operation
nuxt-viewport copied to clipboard

Breakpoint is set even though screen width is greater than its query

Open meirroth opened this issue 1 year ago • 25 comments

Hi! I'm working on integrating this module into our prod site, and trying to align with our current breakpoint usage I ran into the following issue where xl breakpoint is set even when screen size is greater than its query, for example viewport.match('xl') is true on screen width 1600px while its query is (min-width: 1279px) and (max-width: 1536px) I'd like $xl to be false on screen sizes greater than 1536px. Any ideas how to achieve that? See minimal repro: https://stackblitz.com/edit/github-gm83xj?file=plugins%2Fmixins.ts

Thank you for this great module!

meirroth avatar May 01 '24 14:05 meirroth

@meirroth Hi. Oh, I though you have tested with your case.. Okay, I'll need some time to fix that.

mvrlin avatar May 01 '24 14:05 mvrlin

@mvrlin I didn't catch this edge case 🤦‍♂️

meirroth avatar May 01 '24 15:05 meirroth

@meirroth I created a branch for you https://github.com/mvrlin/nuxt-viewport/tree/max-width, try locally please, that should work

mvrlin avatar May 01 '24 15:05 mvrlin

@mvrlin I tired your branch and it changed the media query of xl from (min-width: 1279px) and (max-width: 1536px) to (min-width: 1279px), It didn't fix the issue. Not even sure there is an issue with the queries. If you look at the repro link I shared above, the media queries look fine.

meirroth avatar May 01 '24 15:05 meirroth

@mvrlin I found a temporary fix, by adding a dummy breakpoint to the end https://stackblitz.com/edit/github-gm83xj-gaurep?file=nuxt.config.ts

meirroth avatar May 01 '24 15:05 meirroth

@meirroth What's wrong with my solution? When you set max-width: N, greater resolutions then the edge one will not be trigger, so because of that I made the last query the minimum of the penultimate

mvrlin avatar May 01 '24 16:05 mvrlin

@mvrlin Thank you for taking the time to investigate this issue! Your solution didn't solve the issue initially described. It replaced the media query of xl from being (min-width: 1279px) and (max-width: 1536px) to (min-width: 1279px) with my config, which isn't right. xl should have a max-width query just like all other breakpoints, and it should be set to the N defined in the config. Setting xl: 1536 currently produces media query (min-width: 1279px) and (max-width: 1536px) which is correct! It's the $viewport.breakpoint and .match() that are wrong, when screen is greater than the last breakpoint. Let me know if that makes sense.

meirroth avatar May 01 '24 16:05 meirroth

@meirroth image

Please try to test it with window.matchMedia

mvrlin avatar May 01 '24 16:05 mvrlin

@mvrlin That perfectly demonstrates the issue, thanks for the tip. See how on screen width 1600px window.matchMedia("(min-width: 1279px) and (max-width: 1536px)").matches evaluates to false, unlike this module which returns true from viewport.match('xl'). I would expect viewport.match('xl') to return false in this case. You see what I mean?

meirroth avatar May 01 '24 16:05 meirroth

@meirroth Yes, but the issue here is that it sets the value based on the last truthy match. If you try to run every media query, you will see that on resolutions greater than 1536 every media query will be false

mvrlin avatar May 01 '24 16:05 mvrlin

@mvrlin So do you agree that viewport.match('xl') should return false, and viewport.breakpoint should be null on resolutions greater than 1536, where xs: 1536 is configured?

meirroth avatar May 01 '24 16:05 meirroth

@meirroth In this case yes, but you will be left with no breakpoint then, or as you did previously only by setting some large number which is not ideal

mvrlin avatar May 01 '24 17:05 mvrlin

@mvrlin I think that makes most sense, because it's consistent with how CSS media queries work.

meirroth avatar May 01 '24 17:05 meirroth

@meirroth If you want the $xl to false, change it from:

return this.viewport.isLessThan('xl') || this.viewport.match('xl');

to:

return this.viewport.match('xl');

mvrlin avatar May 01 '24 17:05 mvrlin

@mvrlin That doesn't help. As I said before, viewport.match('xl') returns true on resolutions greater than 1536, where xs: 1536 is configured. I think that's a bug, considering the query for xl is (min-width: 1279px) and (max-width: 1536px) and window.matchMedia("(min-width: 1279px) and (max-width: 1536px)").matches evaluates to false.

meirroth avatar May 01 '24 17:05 meirroth

@meirroth Are you referring to the latest release or the branch that I made for you?

mvrlin avatar May 01 '24 18:05 mvrlin

@mvrlin Both.

meirroth avatar May 01 '24 18:05 meirroth

@meirroth Yes, but the issue here is that it sets the value based on the last truthy match. If you try to run every media query, you will see that on resolutions greater than 1536 every media query will be false

@meirroth This is because of that. You are switching between breakpoints and 1600px doesn't fit with any.

mvrlin avatar May 02 '24 06:05 mvrlin

@mvrlin Right, so should it return null or undefined instead of keeping the last breakpoint?

meirroth avatar May 02 '24 09:05 meirroth

@meirroth I don't think that it should return nullable value because that could break something easily.

mvrlin avatar May 02 '24 10:05 mvrlin

@mvrlin Fine, so not sure if you want to change something in the module or not. For now I've deployed this workaround to prod, and it seems to work 😏

@mvrlin I found a temporary fix, by adding a dummy breakpoint to the end https://stackblitz.com/edit/github-gm83xj-gaurep?file=nuxt.config.ts

meirroth avatar May 02 '24 12:05 meirroth

@meirroth I'm not sure what do you expect the breakpoint to be, because it cannot be nullable by design 😅

mvrlin avatar May 02 '24 12:05 mvrlin

@meirroth I don't fully understand why it can't be nullable by design (can you please explain?), but I think it doesn't make sense for it to not match the media query, as described before:

... viewport.match('xl') returns true on resolutions greater than 1536, where xs: 1536 is configured. I think that's a bug, considering the query for xl is (min-width: 1279px) and (max-width: 1536px) and window.matchMedia("(min-width: 1279px) and (max-width: 1536px)").matches evaluates to false.

meirroth avatar May 02 '24 12:05 meirroth

@meirroth Because if so there is something wrong with the query or the device

mvrlin avatar May 02 '24 12:05 mvrlin

@mvrlin Got it. If you agree with "it doesn't make sense for it to not match the media query" then what do you suggest we do to fix it? Otherwise, we can close this issue and perhaps we mention this edge case in the README for those using maxWidth?

meirroth avatar May 02 '24 15:05 meirroth

@mvrlin hey 👋 what's the latest on this?

meirroth avatar Sep 21 '24 18:09 meirroth

@meirroth Hi! I decided to close the issues that were hanging here. Yes, I will mention about the edge case in the README.

mvrlin avatar Sep 22 '24 10:09 mvrlin

@mvrlin Thanks!

meirroth avatar Sep 24 '24 20:09 meirroth