Key metrics layout broken at 960px

[aaemnnosttv]

Bug Description

The key metrics setup CTA has a broken layout between 960-974px

Steps to reproduce

1. Use a fresh site
2. Go to SK dashboard after connecting GA (with data)
3. See KMW setup CTA
4. Adjust browser width to 960px up to ~974px
5. See below

Note that to reliably reproduce the issue it may be necessary to view the site using a browser with always-visible scrollbars.

If using a Mac, set the system setting "Show scroll bars" to "Always", and when using Chrome's responsive devtool view
ensure the device type is set to Desktop ensure the scrollbar is present.

Screenshots

---

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

• The layout should be adjusted to display as designed across viewports

Implementation Brief

• Provide a version of the useWindowWidth() hook that uses global.innerWidth instead of document.documentElement.clientWidth to determine the window width.
• Replace usage of the currently used hook with the new one throughout the codebase.
• See the PoC PR PoC bugfix for broken setup CTA layouts #9996, with additional details in this comment below: Key metrics layout broken at 960px #9911 (comment).

Test Coverage

• Add test coverage for the new useWindowWidth() hook.
• Fix any failing tests.

QA Brief

1. Use a fresh site
2. Go to SK dashboard after connecting GA (with data)
3. See KMW setup CTA
4. Adjust browser width to 960px up to ~974px. The banner should not break in these widths.

Changelog entry

• Fix key metrics setup CTA layout at 960px.

[benbowler]

Hey @aaemnnosttv I couldn't repeat this today in develop and main, testing both in Safari, Firefox and Chrome.

Screen.Recording.2024-12-18.at.09.32.11.mov

Are there any other conditions to repeat the issue? Is this only in the dist build of the plugin?

[zutigrm]

I am unable to reproduce this either. Unassigning myself in order not to trap the issue if someone else wants to give it a try. And I will be on PTO for the first 2 days of the next week as well

[10upsimon]

Took a look myself to be triple sure of the findings by @zutigrm and @benbowler and I also do not see the issue on latest develop, tried 960, 970 and 974 width:

• 960px:

• 970px:

• 974px:

[techanvil]

Hey @10upsimon, reviewing your comment above I recall that in my testing I was only able to reproduce the issue when I actually resized the browser window, rather than resizing via the devtools responsive capability, as mentioned in Slack.

You might want to give this another try to see if that lets you reproduce this; if not, or if you no longer have capacity for this one, feel free to assign it back to me.

[10upsimon]

Thanks @techanvil . I am, however, still unable to reproduce the issue with manual resize. This applies to Chrome, Firefox and Safari on Mac OS Sequoia. Please see attached Gif's showcase resize across said browsers:

Chrome

https://p200.p0.n0.cdn.zight.com/items/llueJdxp/fd19466a-e90a-44e0-ae4d-f29ee5a24010.gif

Firefox

https://p200.p0.n0.cdn.zight.com/items/v1uGeQoG/b59e6506-24b5-4238-8269-6528b5a98007.gif

Safari

https://p200.p0.n0.cdn.zight.com/items/7KuwmkBn/5cc17222-f573-4419-b2b9-6e45d8411f91.gif

[techanvil]

Thanks for the update @10upsimon. Having picked this up again and investigated further I've been able to identify a fix.

However, I'd like to ask @aaemnnosttv to take a look at my findings.

I've only been able to reproduce this on browsers which have an always-visible scrollbar. Initially, I noticed this while testing on my Linux workstation, but I've been able to reproduce it on a Mac as well by toggling the "show scroll bars" system setting to "always".

We are currently using the useWindowWidth() hook provided by @react-hook/window-size/throttled to determine our breakpoints in JS. This hook usesdocument.documentElement.clientWidth to determine the window width, which gives the viewport width without the scrollbar (i.e. it's a a bit narrower than window.innerWidth when a scrollbar is always visible). This in turn is giving us unexpected breakpoints vis à vis the actual window size when the scrollbar is present. Providing an updated version of useWindowWidth() that uses window.innerWidth instead fixes the issue on browsers with an always-visible scrollbar, and should be a transparent change in browsers that don't show a scrollbar.

Note that the Audience Segmentation setup CTA banner is also showing the wrong graphic at the identified viewport range, and this is also fixed by the change outlined above.

Currently using document.documentElement.clientWidth:

Updated to use window.innerWidth:

This seems like a reasonable change to make. However, @aaemnnosttv, as you were also able to reproduce the issue but seemingly in different circumstances, please can you take a look and see if the fix I've identified is applicable to your scenario as well? I'd also be Interested to hear your thoughts in general on the suggested fix.

Here a PoC PR for the fix: #9996

[aaemnnosttv]

Thanks @techanvil. I've done a bit more investigation here as well and I think there are some issues with our breakpoints which are being applied differently in JS and CSS. That's probably not a surprise though and there are likely more fundamental changes needed to address this but that is probably better suited to a separate issue.

Yup, this is quite a long standing, wide reaching cause of dissonance in the codebase that it would be great to address. I've created a new issue to track this, #10088, although as mentioned in the issue, we'll undoubtedly want to expand this into multiple issues, possibly even a new epic.

Sounds good, glad we finally have an issue to address it ❤

Are you sure @react-hook and react-use are by the same author? It doesn't look that way, they are on different GitHub accounts which don't appear to be related, unless I've missed something:

• streamich/react-use
• jaredLunde/react-hook

Hmm, not sure how I mixed that up but thanks for the correction!

I am not entirely sure we need to remove @react-hook (the repo is still being maintained, although useWindowWidth() hasn't had an update for a while), but it would be nice not to be using two hook utility packages, and we certainly can remove it without any problems by the look of it - we're only directly using two hooks from it in Site Kit, useWindowWidth() and useMergedRef(). As we've identified, useWindowWidth() is not a good fit for our use case, while useMergedRef() is still useful AFAICT but we could for probably swap it out for the more popular (and more recently updated) merge-refs. I imagine we'll be removing useWindowWidth() as part of this issue, so I could create a separate one to swap out useMergedRef() for merge-refs - WDYT?

Sure, from what I remember I was looking at a repo that hadn't been updated in ~8 years. I can't find it now, but there is this which is what I found initially https://www.npmjs.com/package/react-hook which is pretty close.

I'm happy to keep the package if it's maintained and there's good reason to use it over another.

Changing to use the newer hook seems to work well but probably needs more investigation along with changes to the setup component.

Actually, I'm a bit hesitant to switch to the react-use hook; there are a couple of differences between it and the @react-hook version although one can be worked around, and we might decide we don't mind about the other.

1. The react-use version doesn't throttle the window resize event, whereas the @react-hook version is throttled (with a default of 30fps). We should be able to work around this by passing a throttled handler into the hook, but we'd need to check this works.
2. The react-use hook only listens to the resize event, whereas the @react-hook version listens to both resize and orientationchange. This might not be a problem, but it does suggest the resulting behaviour could change on mobile devices.

So, maybe we can use react-use, but I'm leaning toward providing our own implementation to better match the current behaviour. WDYT?

We should probably keep the throttling, and orientationchange seems like something we'd want to keep as well. We could still use the hook within our own wrapper hook, could we not? I'm not against implementing our own, but ideally we don't have to reinvent it.

One final observation is that useBreakpoint does not include the breakpoint value in the condition, which means a width of 960 won't be considered desktop, when it is the starting point width for that size. In CSS, min and max are both inclusive, so given this hook is defined as cascading min-widths, the values should also be inclusive to match CSS. This would also cause unexpected behaviors on widths that fall exactly on media query boundaries (such as 960 here).

This is another aspect to consider that I've included in #10088.

Great, thank you 👍

---

IB ✅

[techanvil]

Thanks @aaemnnosttv!

I am not entirely sure we need to remove @react-hook (the repo is still being maintained, although useWindowWidth() hasn't had an update for a while), but it would be nice not to be using two hook utility packages, and we certainly can remove it without any problems by the look of it - we're only directly using two hooks from it in Site Kit, useWindowWidth() and useMergedRef(). As we've identified, useWindowWidth() is not a good fit for our use case, while useMergedRef() is still useful AFAICT but we could for probably swap it out for the more popular (and more recently updated) merge-refs. I imagine we'll be removing useWindowWidth() as part of this issue, so I could create a separate one to swap out useMergedRef() for merge-refs - WDYT?

Sure, from what I remember I was looking at a repo that hadn't been updated in ~8 years. I can't find it now, but there is this which is what I found initially https://www.npmjs.com/package/react-hook which is pretty close.

I'm happy to keep the package if it's maintained and there's good reason to use it over another.

Although I was suggesting we could swap it out, keeping useMergedRef() seems fine to me too, given that it's an isolated package with no dependencies. It hasn't been updated for a while, but it's a simple hook, and evidently not something that has needed ongoing maintenance. So, sure, let's keep things simple and stick with this one for the time being at least.

Actually, I'm a bit hesitant to switch to the react-use hook; there are a couple of differences between it and the @react-hook version although one can be worked around, and we might decide we don't mind about the other.

1. The react-use version doesn't throttle the window resize event, whereas the @react-hook version is throttled (with a default of 30fps). We should be able to work around this by passing a throttled handler into the hook, but we'd need to check this works.
2. The react-use hook only listens to the resize event, whereas the @react-hook version listens to both resize and orientationchange. This might not be a problem, but it does suggest the resulting behaviour could change on mobile devices.

So, maybe we can use react-use, but I'm leaning toward providing our own implementation to better match the current behaviour. WDYT?

We should probably keep the throttling, and orientationchange seems like something we'd want to keep as well. We could still use the hook within our own wrapper hook, could we not? I'm not against implementing our own, but ideally we don't have to reinvent it.

Do you mean using the react-use version of useWindowWidth() in our own hook? I'd say that yes, it makes sense if we just want to retain the throttling because we should be able to pass a throttled function in as the handler, however as we do want to retain the orientationchange handling as well, I'm not sure it's such a good fit because we'll need to provide our own throttled event handling for that anyway, so we might as well just stick with one bespoke piece of logic that handles both the resize and orientationchange events...

[wpdarren]

@mohitwp @kelvinballoo for whoever picks up this ticket. It's worth noting that the QAB is lacking some important details which are included in the description of the ticket. I know we like to recreate the issue in this scenario and then switch to develop to make sure it has been fixed.

Note that to reliably reproduce the issue it may be necessary to view the site using a browser with always-visible scrollbars. If using a Mac, set the system setting "Show scroll bars" to "Always", and when using Chrome's responsive devtool view ensure the device type is set to Desktop ensure the scrollbar is present.

Thought it was worth highlighting here.

[mohitwp]

QA Update ⚠

• Tested on dev environment.
• Tested on Mac Safari using Browserstack and on Chrome browser.
• Verified Key metrics CTA design is now not getting break on viewports having width to 960px up to ~974px.
• Verified Key metrics CTA design in desktop, mobile and tablet viewports.

Question -
I am able to reproduce the issue in the latest environment using the Chrome browser. However, the issue only appears for a few seconds during page load. I verified this on the development environment and can still reproduce the issue while the page is loading. Once the page finishes loading, the design no longer breaks. I will ask @wpdarren or @kelvinballoo if they are able to reproduce the actual issue.

screencast -Latest environment

Recording.1753.mp4

screencast - Dev environment

Recording.1755.mp4

Pass Cases

Chrome Browser

Recording.1752.mp4

Safari browser

Recording.1751.mp4

iPad viewports

Mobile Viewport

[kelvinballoo]

QA Update ✅

Was able to simulate it on my Mac OS on Chrome, Chrome emulator and Firefox.
After switching to develop branch, the issues are no more.

9911.test.mov