primer / brand

React components and Primitives for GitHub marketing websites
https://primer.style/brand
MIT License
69 stars 31 forks source link

Improvements to `useWindowSize` hook #788

Open joshfarrant opened 2 hours ago

joshfarrant commented 2 hours ago

There are a couple of things which could be improved in the useWindowSize hook. The most notable is the behaviour of the is[Size] booleans returned by the hook.

The current behaviour of the hook is, in my opinion, slightly counter-intuitive. For example, isSmall will return true when the viewport width is any size greater than the small breakpoint. I'd expect isSmall to only return true when the viewport width is greater than or equal to the small breakpoint, and less than the medium breakpoint. Eg

- isXSmall: window.innerWidth >= 320,
- isSmall: window.innerWidth >= 544,
- isMedium: window.innerWidth >= 768,
- isLarge: window.innerWidth >= 1012,
- isXLarge: window.innerWidth >= 1280,
+ isXSmall: window.innerWidth >= 320 && window.innerWidth < 544,
+ isSmall: window.innerWidth >= 544 && window.innerWidth < 768,
+ isMedium: window.innerWidth >= 768 && window.innerWidth < 1012,
+ isLarge: window.innerWidth >= 1012 && window.innerWidth < 1280,
+ isXLarge: window.innerWidth >= 1280 && window.innerWidth < 1440,
isXXLarge: window.innerWidth >= 1440,

I can see the merits of the existing behaviour, but it caught me out the first time I looked at the hook, as it did @victoriaNine (link to Slack conversation).

Another potential improvement for the hook would be to have it use a context to ensure that only one 'resize' event listener is added to the window, regardless of how many times the hook is called. At the moment, every time the hook is called a new listener is added to the window, which is a bit redundant.

simurai commented 2 hours ago

For "smaller than" (<), the values would need to be 1px less. E.g. < 543, < 767 etc.. Otherwise when the viewport is for example exactly 544, both isXSmall + isSmall would be true.