Make the getHeaderHeightWithoutNav() function more defensive.
#6672
Conversation
|
Size Change: +681 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
|
Closing for now as we'll need to create a separate issue to resolve this. |
There was a problem hiding this comment.
Thanks @techanvil – one more thing I wanted to mention is that we might also want to consider adding something where its used in case the function were to change there where this could be reintroduced. Perhaps just adding a test or two to guard against a regression here would be a good idea.
site-kit-wp/assets/js/components/notifications/BannerNotification/index.js
Lines 144 to 146 in 7accb8e
| * @param {string} breakpoint The current breakpoint. | ||
| * @return {number} The height of the sticky WordPress admin bar, if present. | ||
| */ | ||
| function getWordPressAdminBarHeight( breakpoint ) { |
There was a problem hiding this comment.
Regarding the use of a named breakpoint, are we sure our references in JS are aligned with the same in CSS? I realize the use here dates back quite a few releases but it seems a bit strange to rely on that when we're querying for elements and all. It could be that the current errors may only happen on the exact boundary of a breakpoint (we've seen some odd things happen at 600px before specifically for this reason).
There was a problem hiding this comment.
I can confirm the BREAKPOINT_SMALL / BREAKPOINT_TABLET crossover does correlate precisely to the breakpoint at which the WordPress admin bar changes its stickiness.
However, it's a valid concern anyway, I think, considering that useBreakpoint() uses a throttled value from useWindowWidth(), which means it could be using a stale value when determining the header height...
I think the additional defensive measures being proposed here would be enough to mitigate this edge case, though.
assets/js/util/scroll.js
Outdated
| // Otherwise, we use the value of the bottom of the Site Kit header's bounding box as this will take into account the sticky WordPress admin bar. | ||
| return breakpoint === BREAKPOINT_SMALL | ||
| ? header.offsetHeight | ||
| : header.getBoundingClientRect().bottom; |
There was a problem hiding this comment.
Since .bottom could be negative here, should we have an additional condition to return 0 in that case to avoid returning a negative number here?
There was a problem hiding this comment.
Hmm - the way this way originally written, there was a check for a negative result in the caller, to catch unexpected cases where the header is non-sticky. However, as you've pointed out we can make a more reliable check for stickiness in the caller, I think it would be a good idea to simply assume the header is sticky within this function - but, do what you've suggested just to handle the unexpected case here, i.e. do return 0 if .bottom is negative.
assets/js/util/scroll.js
Outdated
| @@ -81,24 +122,26 @@ export function getHeaderHeightWithoutNav( breakpoint ) { | |||
| '.react-joyride__overlay' | |||
There was a problem hiding this comment.
Another potential enhancement would be to use body.googlesitekit-showing-feature-tour here since this is where the actual style to make it not sticky is, in addition to being our own class :)
Alternatively, perhaps we can use other queries to better determine whether the header is sticky or not? i.e. the page 0 < scrollY and the header's top == 0 ?
There was a problem hiding this comment.
Aha - thanks for pointing that out, I hadn't noticed we're explicitly setting the position to be non-sticky, and had assumed it was some DOM-related aspect, although hadn't dug into it (obviously!).
Given this is the case, I'd suggest we simply get the calculated style for the header and check whether the position property is sticky. I'm interested in your idea of using scrollY and top but that could give some indeterminate results (say both scrollY and top are 0, we wouldn't know if the header is sticky or not).
There was a problem hiding this comment.
I guess my thought was to make the functions more about the position of the header in the viewport rather than specific positioning rules. It's also worth noting that the WP admin bar does not use position:sticky but fixed so there is more than one way to achieve that kind of position. No problem in checking for sticky on our own component since we own that style, but it might be simpler to think about it independent of that.
There was a problem hiding this comment.
@aaemnnosttv, I do take your point, but I can't think of how we can determine the stickiness of the header purely based on its position in the viewport, or otherwise change the logic to avoid needing to make this determination.
Unless you've got any suggestions on how to achieve this, we may have no better alternative to checking the CSS position property...
| * @param {string} breakpoint The current breakpoint. | ||
| * @return {number} The height of the sticky Site Kit header including the sticky WordPress admin bar when it's present. | ||
| */ | ||
| function getGoogleSiteKitHeaderHeight( breakpoint ) { |
There was a problem hiding this comment.
This function could be misinterpreted to be the same as getHeaderHeight above if using by name.
There was a problem hiding this comment.
Yes, good point. I would suggest changing the exported functions to getStickyHeaderHeight() and getStickyHeaderHeightWithoutNav() to be a bit more explicit, while these non-exported functions can remain as they are...
Yes, I think it's worth adding a guard at the point of use, as a failsafe. I'd suggest calling We could add some tests, too, I think we should be able to test this one using JSDom... :) |
|
I do agree, it's really just for consistency I suggested that approach, given we already do that in a similar scenario in Hmm. Well, arguably it is going a bit overboard to check the value in two places, but I've created a utility function, Obviously there's still room for manoeuvre here, please see what you think. |
techanvil
force-pushed
the
bug/6109-incorrect-view-notifications-fix
branch
from
282ee05 to
f764e5e
Compare
Summary
Addresses issue:
getHeaderHeightWithoutNav()more robust to avoid errors seen in the wild. #6674Relevant technical choices
Having seen some related error reports from production sites, this PR adds some defensive programming to the
getHeaderHeightWithoutNav()function.PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist