feat(pfe-navigation): report pfe-nav's height in a global CSS variable

[mwcz]

What has changed and why

pfe-navigation will now create a global CSS variable containing the pfe-navigation element's height. @chrisdo1 requested this so that a sticky sub-nav can be correctly positioned below the pfe-navigation element. The height can also be used to calculate a vertical offset for anchor links on pages where pfe-navigation is sticky.

• --pfe-navigation--ReportedHeight: when pfe-navigation initializes, it will create a global (on body) CSS variable that contains the height of the pfe-navigation item. The primary uses for this is calculating the offset for anchor links, and for positioning a sticky sub-header below the pfe-navigation. Note that multiple pfe-navigation elements will write the same variable, unless...
• --pfe-navigation--ReportedHeight-${ID}: like the previous varaible, but if the pfe-navigation has an id attribute, it will be appended to the CSS variable name. This makes it possible to distinguish the heights of multiple pfe-navigation elements on the same page.

@castastrophe, what do you think of these variable names? I don't think they're proper BEM because ReportedHeight isn't a property. I didn't want to use "Height" beccause I want it to be clear that this is a read-only property, and that overridding it will have no effect.

Testing instructions

Automated tests are included in the PR. You can also;

1. Include pfe-navigation on a page and see if it creates the variable.

Ready-for-merge Checklist

• Expected files: all files in this pull request are related to one feature request or issue (no stragglers)?
• Did you update or add any necessary documentation (README.md, WHY.md, etc.)?
• Was this feature demo'd and the design review approved?
• Did you update the CHANGELOG.md file with a summary of this update?

[castastrophe]

Great idea! Hmm how about --pfe-navigation--Height--actual and --pfe-navigation__${ID}--Height--actual? I think that’s a bit more BEMy. Is it better to know the ID or the index of the navigation? Like this is the second navigation? If it’s not sticky does this go to 0 when the nav is no longer visible?

[mwcz]

Great idea! Hmm how about --pfe-navigation--Height--actual and --pfe-navigation--${ID}--Height--actual? I think that’s a bit more BEMy.

Sounds good to me! Change made.

Is it better to know the ID or the index of the navigation? Like this is the second navigation?

I think it's better to use the ID, or a new attribute (pfe-height-namespace). Index can change. Code like this...

top: var(--pfe-navigation--2--Height--actual)

would break if the ordering changes or navs are added/removed, but this is resilient:

top: var(--pfe-navigation--main-menu--Height--actual)

If it’s not sticky does this go to 0 when the nav is no longer visible?

No, I didn't consider that scenario. Is that the right thing to do? Might it be better to set it to 0 any time the nav isn't sticky?

[castastrophe]

@mwcz Instead of WIP, going forward could you open a draft PR? https://github.blog/2019-02-14-introducing-draft-pull-requests/

[castastrophe]

Since the ID represents a region, I think it should be underscore prefixed instead: --pfe-naviation__main-menu--Height--actual?

[castastrophe]

Also need to determine functionality when the navigation is not sticky.

[castastrophe]

Suggested change

	- `--pfe-navigation--ReportedHeight-${ID}`: like the previous varaible, but if the pfe-navigation has an `id` attribute, it will be appended to the CSS variable name. This makes it possible to distinguish the heights of multiple pfe-navigation elements on the same page.
	- `--pfe-navigation__${ID}--Height--actual`: like the previous variable, but if the pfe-navigation has an `id` attribute, it will be appended to the CSS variable name. This makes it possible to distinguish the heights of multiple pfe-navigation elements on the same page.

[chrisdo1]

@castastrophe updated lines 72 and 73 to show the correct variable name.

[castastrophe]

Suggested change

	`--pfe-navigation--${this.id ? `${this.id}--` : ""}Height--actual`;
	`--pfe-navigation__${this.id ? `${this.id}--` : ""}Height--actual`;

[chrisdo1]

@castastrophe replaced -- with __

[chrisdo1]

@castastrophe @mwcz looking at this again, if there is no ID, the resulting custom property will look like this:
--pfe-navigation__Height--actual

Is that correct or should it be --pfe-navigation--Height--actual in which case we'd need to move the __ into the ternary?

[starryeyez024]

When there is a main navigation and a secondary (horizontal) navigation on a page, I think there are two mutually exclusive ways that you would want them to interact:

1. the main nav and the sticky nav are in the same parent container, and so you want to offset the secondary nav so its ~72px lower because thats the height of the main nav top: var(--nav-offset). If the parent container, let's say article scrolls off the screen, then the main navigation and they secondary nav would both scroll offscreen too, because that's how position sticky works (so there's no chance of a gap appearing when only the secondary nav is visible, and its offset from the top of the screen).
2. the secondary nav is in a different parent container than the main nav. In this case, when the new parent container is scrolled into view, it pushes the main nav off the screen, like this. In this case, the secondary nav would not use the main-nav offset variable. The secondary nav would just be top: 0

In my mind, this PR is intended to help solve for use case #1, therefore the value of the main-menu-offset variable can remain constant.

[kylebuch8]

Limitless Guitarist: Tom Morello