Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX: restore docked body document from glimmer site header #27003

Merged
merged 1 commit into from
May 13, 2024

Conversation

tyb-talks
Copy link
Contributor

@tyb-talks tyb-talks commented May 13, 2024

Follow up on #26967 - that earlier fix didn't actually fix the issue. Added a simple test to ensure this fix actually works this time 😅 .

Main bug:

In the dockCheck function for both glimmer and widget site-headers, we only set the _docAt property if it's strictly equivalent to null i.e. if this._docAt === null .

However, that property is initialized to different values in both versions of the site-header:

// in widget header
    docAt: null,

// in glimmer header - glimmer-site-header.gjs
  _docAt; // this is undefined

So we never set _docAt to an integer in the glimmer header... and comparing any integer to undefined always returns false in Javascript. (funnily enough, comparing any positive integer to null returns true... yes Wat).

More on the dockCheck function to determine if we should set the docked class name:

Besides the above bug, it appears that in our widget site header, we've always had the site header perma-docked (see any sites still using the widget header and look at the body element). The current offset checking logic in dockCheck:

 const main = (this._applicationElement ??=
      document.querySelector(".ember-application"));
    const offsetTop = main?.offsetTop ?? 0;
    const offset = window.pageYOffset - offsetTop;
    if (offset >= this._docAt) {
      // set docked
    } else {
      // unset docked
    }

Since the main element's offsetTop will always be 0 (you can try this out in console regardless of current scroll position), that offset variable is always window.pageYOffset which cannot be less than this._docAt (this.headerElement.offsetTop), and therefore we never reach the scenario where we will set docked to false.

That said, I'm unsure if this was intentionally left as-is in the glimmer site-header, so will leave it for now.

@tyb-talks tyb-talks force-pushed the fix-set-body-docked-class-in-site-header branch from fd809e3 to d2d727a Compare May 13, 2024 15:47
@@ -243,7 +243,7 @@ export default class GlimmerSiteHeader extends Component {

@bind
dockCheck() {
if (this._docAt === null) {
if (this._docAt === undefined || this._docAt === null) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wanted to use if (!Boolean(this._docAt)) which is more succinct but got hit by the no-extra-boolean-cast eslint rule. !this._docAt is not quite what this conditional is shooting for as that lets through the 0 value as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW Boolean(0) is also false, so that would also let through the 0 value. So I think the linter is correct in this case. I wish there were some kind of 'nullish' check built in to JS 😢

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh 😫 Javascript why

@davidtaylorhq
Copy link
Member

we never reach the scenario where we will set docked to false

I think it happens when custom themes introduce content above the header. e.g. https://forums.wyze.com/

@tyb-talks tyb-talks merged commit 2df4f38 into main May 13, 2024
16 checks passed
@tyb-talks tyb-talks deleted the fix-set-body-docked-class-in-site-header branch May 13, 2024 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants