feat(GlobalHeader): add getGlobalHeaderHeight helper function #1417

christian-dinh · 2021-02-17T22:07:37Z

Overview

There are two different ways I tried to do this, both visible in two commits. the first (with refs) is fancier but probably unnecessarily so. It uses refs and resize listeners inside of useLayoutEffect to function agnostically from how the header is sized. The second (without refs) simply sets the height based on useBreakpointAtOrAbove and uses it for both the imperative handle and the sizing of the header itself. Both use useImperativeHandle, but the latter way seems better to me so that's what the main diff is. If you look at the first commit on its own and prefer it I am open to discussion! Also open to alternatives to an imperative handle if anyone has a better idea.

Adds a ~~hook~~ helper function for getting the current height of the GlobalHeader for layouts that require that info. Includes docs and unit tests!

PR Checklist

Related to designs:
Related to JIRA ticket: REACH-619
I have run this code to verify it works
This PR includes unit tests for the code change

codecov · 2021-02-17T22:07:44Z

Codecov Report

Merging #1417 (468fb49) into main (e736bce) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1417      +/-   ##
==========================================
+ Coverage   82.23%   82.28%   +0.04%     
==========================================
  Files         316      318       +2     
  Lines        2545     2552       +7     
  Branches      663      663              
==========================================
+ Hits         2093     2100       +7     
  Misses        399      399              
  Partials       53       53

Impacted Files	Coverage Δ
...gamut-labs/src/experimental/GlobalHeader/consts.ts	`100.00% <100.00%> (ø)`
...gamut-labs/src/experimental/GlobalHeader/index.tsx	`100.00% <100.00%> (ø)`
.../gamut-labs/src/experimental/GlobalHeader/util.tsx	`100.00% <100.00%> (ø)`

packages/gamut-labs/src/experimental/GlobalHeader/index.tsx

christian-dinh · 2021-02-18T18:48:21Z

this is still a draft, final version will have unit tests if this looks like a reasonable add

packages/gamut-labs/src/experimental/GlobalHeader/index.tsx

MattSchiller · 2021-02-19T16:36:43Z

packages/gamut-labs/src/experimental/GlobalHeader/index.tsx

+// For performance reasons, this component renders the mobile and desktop versions
+// of itself simultaneously and switches between them using css (instead of using
+// any of the useBreakpoint hooks, which rely on js for the same behavior).


rogervera

LGTM. Thanks Chris for taking the time to work out and comminucate re: the best solution.

It was discussed in this thread:
https://github.com/Codecademy/client-modules/pull/1417/files#r577980940

I too am curious about the performance impact useBreakpointAtOrAbove does have vs rendering both versions and using css to toggle.

If the overhead is unnecessary given the need, I wonder if we should audit and look into just replacing current use of useBreakpointAtOrAbove (and marking as deprecated) across the monolith/static-sites and replace it with CSS. I think in most cases, it's just for this mobile v desktop view need on a single component.

JoshuaKGoldberg

Err, what about react-anchor-link-smooth-scroll? Can you show an example in the monolith of needing this JS hook?

JoshuaKGoldberg · 2021-02-19T17:15:21Z

@rogervera yeah that was my original concern with the hooks and why I was so dubious about their need 😄 . https://codecademy.atlassian.net/browse/TR2-394 -> https://codecademy.atlassian.net/browse/REACH-475

Conclusion: most of the time, these hooks are not a good idea to use in components that are visible on page load.

christian-dinh · 2021-02-19T17:24:19Z

@JoshuaKGoldberg not the monolith, but static sites:

https://github.com/codecademy-engineering/static-sites/blob/main/src/templates/LandingPage/helpers/index.tsx#L101-L107

would help us get rid of the hardcoded 80, which is incorrect on mobile displays on top of being mysterious and fragile 👻. I think we would still need this value to do the same thing with react-anchor-link-smooth-scroll since you gotta set the offset prop

packages/gamut-labs/src/experimental/GlobalHeader/hooks.tsx

JoshuaKGoldberg · 2021-02-21T20:57:21Z

packages/gamut-labs/src/experimental/GlobalHeader/index.tsx

-    <Box display={{ base: 'none', md: 'block' }} height="80">
+    <Box
+      display={{ base: 'none', [breakpoint]: 'block' }}
+      height={desktopHeight.toString()}


@codecaaron I wonder if we could expand the types for height and width to allow number | string, so folks didn't have to manually toString()?

Sure! It technically will already handle numbers with no units cleanly so it should just be a matter of updating the default types

JoshuaKGoldberg · 2021-02-21T20:59:44Z

packages/styleguide/stories/Labs/Experimental/Molecules/GlobalHeader.stories.mdx

 2. Update `GlobalHeaderVariants.tsx` to include the new item for the user roles that need it.
 3. Update tests in `GlobalHeader-test.tsx`.
+
+### How to Access GlobalHeader's height


@christian-dinh about the resize situation: are there situations other than scrolling to a specific element that would care about the header height? I wonder if we could avoid the fragile number variables by using a ref after all, but exposing a scrollPastHeader function as part of a React Context instead of a hook...

I think this could be used for the SkipToContent targets which need to offset the sticky header height. The monolith currently has a headerHeight variable set to 4rem here and utilizes it for a negative margin top for the target here. Static sites is currently just hard coding the -4rem here.

It's also possible that we would want to forego this pathway entirely in favor of css variables if we only want this information for CSS rules.

Like, we can include a base sass file with something like:

:root { --global-header-height: 64px; } @media screen and (max-width: 800) { // or whatever our mobile breakpoint is { :root { --global-header-height: 80px; } }

And then we don't expect our components to receive this information through React (JS) at all. We just update their sass stylings to use these variables directly.

If we only want these values for css purposes, then I think this accomplishes the best of both worlds (aka, The Hannah Montana Solution), where we never have the flash of incorrect styling because SSR doesn't have a width value. It's always rendered correctly on the first pass from CSS.

Thoughts?

that's a pretty strong proposal, and actually if we expose a ref on the header we should still be able to access the value in js:

getComputedStyle(headerRef.current).getPropertyValue('--global-header-height');

I do think that js visibility is a requirement for header-clearing scroll targets (since scroll-margin-top is not supported in safari), but this feels pretty good to me if it works.

or yeah put it on :root and then we don't need a ref

Gotcha, I get it now (and learned about emotion globals!). Thanks for the explanation!

Would a snapshot of <GlobalHeader />'s <Global /> not be sufficient to confirm that it's providing the values we're looking for? Like, we don't usually test to confirm that CSS is being applied in the way we expect it to, do we? Would a cypress test be helpful if we really want to confirm that they're being applied correctly?

And for the helper, that should be testable, right?

I'm generally inclined to keep these variables at the :root level (for ease of access in css) over using a ref, especially since it seems like we're going to be looking to use this in JS in non-react-ive patterns (imperatively, as you pointed out).

I think what you've got here is especially awesome already. Excited to have this kind of flexibility in our repo!

Totally fine with keeping it at :root for the reasons you mentioned. I was actually specifically having trouble testing the helper, but I think I might have just had a Big Brain Moment™ 🤯 stand by

ok I switched up what the test was testing and I think this feels pretty good now. Still need to update docs but lmk what u think.

Sorry my computer has been out of commission since the weekend. Finally caught up on setting up new on.

I'll take a look at these comments tonight.

@christian-dinh Thank you for all of this! Not only do I think this is some awesome code, but I've loved the conversation about it.

I left only a single nit/question, otherwise, to whatever extent my approval matters, I think this is some Grade A code right here.

packages/styleguide/stories/Labs/Experimental/Molecules/GlobalHeader.stories.mdx

JoshuaKGoldberg

The CSS variable discussion is interesting, but I don't have any other ideas so approving anyway 😄

packages/gamut-labs/src/experimental/GlobalHeader/hooks.tsx

Things are moving and shaking

codecademydev · 2021-02-23T22:11:04Z

📬Published Alpha Packages:

@codecademy/gamut-labs@10.1.1-alpha.468fb4.0
@codecademy/styleguide@29.1.1-alpha.468fb4.0

codecademydev · 2021-02-23T22:13:24Z

🚀 Styleguide deploy preview ready!

https://60357deee5a5d8007d06db80--gamut-preview.netlify.app

Deploy Logs

MattSchiller · 2021-02-23T23:23:20Z

packages/gamut-labs/src/experimental/GlobalHeader/util.tsx

+
+export function getGlobalHeaderHeight(): number {
+  return parseInt(
+    getComputedStyle(document.documentElement).getPropertyValue(


nit: Does this need the optional chaining on document to work on SSR? Or does it fail gracefully as-is? I dunno myself, just wondering!

document won't exist in SSR in the monolith anymore. So we'll want to put this on a preview server for sure. 😄

given the use case I don't see when this function would be called during SSR, though if it is would it be helpful to log a console warning?

I think I like it as -- it should aggressively crash if used in a component's initial render, as a way to tell us to not do that thing 😄

That's a great idea, yea!

christian-dinh · 2021-03-05T16:59:58Z

closing cause https://github.com/Codecademy/client-modules/pull/1444 does all this and more 🎉

christian-dinh added 3 commits February 17, 2021 16:41

with refs

d0efab6

without refs

b6c7be7

update story

e78fb7e

christian-dinh commented Feb 17, 2021

View reviewed changes

packages/gamut-labs/src/experimental/GlobalHeader/index.tsx Show resolved Hide resolved

use hook instead

dc1f75e

MattSchiller reviewed Feb 18, 2021

View reviewed changes

packages/gamut-labs/src/experimental/GlobalHeader/index.tsx Outdated Show resolved Hide resolved

extract hook and consts into separate files, add tests

8b4c419

christian-dinh changed the title ~~feat(GlobalHeader): add imperative height handle~~ feat(GlobalHeader): add useGlobalHeaderHeight hook Feb 18, 2021

christian-dinh marked this pull request as ready for review February 18, 2021 23:38

christian-dinh requested a review from a team as a code owner February 18, 2021 23:38

christian-dinh requested review from a team, codecaaron and jakemhiller and removed request for a team February 18, 2021 23:38

Merge branch 'main' into cd-header-height

1698df5

MattSchiller reviewed Feb 19, 2021

View reviewed changes

rogervera approved these changes Feb 19, 2021

View reviewed changes

JoshuaKGoldberg suggested changes Feb 19, 2021

View reviewed changes

Merge branch 'main' into cd-header-height

456b10d

judahtanthony reviewed Feb 19, 2021

View reviewed changes

packages/gamut-labs/src/experimental/GlobalHeader/hooks.tsx Outdated Show resolved Hide resolved

JoshuaKGoldberg reviewed Feb 21, 2021

View reviewed changes

packages/styleguide/stories/Labs/Experimental/Molecules/GlobalHeader.stories.mdx Show resolved Hide resolved

JoshuaKGoldberg previously approved these changes Feb 22, 2021

View reviewed changes

wip

41ff690

MattSchiller reviewed Feb 23, 2021

View reviewed changes

packages/gamut-labs/src/experimental/GlobalHeader/hooks.tsx Show resolved Hide resolved

:o

468fb49

christian-dinh changed the title ~~feat(GlobalHeader): add useGlobalHeaderHeight hook~~ feat(GlobalHeader): add getGlobalHeaderHeight helper function Feb 23, 2021

MattSchiller reviewed Feb 23, 2021

View reviewed changes

codecaaron mentioned this pull request Feb 26, 2021

feat(ThemeProvider): Adds GamutThemeProvider component and initial Global styles #1444

Merged

4 tasks

katiezutter mentioned this pull request Mar 3, 2021

fix(GlobalHeader): Consolidate animated headers #1454

Merged

4 tasks

codecaaron mentioned this pull request Mar 5, 2021

style(GlobalHeader): Use theme variable for header height #1469

Merged

4 tasks

christian-dinh closed this Mar 5, 2021

feat(GlobalHeader): add getGlobalHeaderHeight helper function #1417

feat(GlobalHeader): add getGlobalHeaderHeight helper function #1417

Uh oh!

Conversation

christian-dinh commented Feb 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

PR Checklist

Uh oh!

codecov bot commented Feb 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

christian-dinh commented Feb 18, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rogervera left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

JoshuaKGoldberg commented Feb 19, 2021

Uh oh!

christian-dinh commented Feb 19, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MattSchiller Feb 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecademydev commented Feb 23, 2021

📬Published Alpha Packages:

Uh oh!

codecademydev commented Feb 23, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christian-dinh commented Mar 5, 2021

Uh oh!

Reviewers

christian-dinh commented Feb 17, 2021 •

edited

Loading

codecov bot commented Feb 17, 2021 •

edited

Loading

rogervera left a comment •

edited

Loading

MattSchiller Feb 22, 2021 •

edited

Loading