fix: add Gatsby theme New Relic global nav to Open Source site mobile views (#556)

[tyreer]

These changes should address: #556

The global nav should now mirror the experience on https://developer.newrelic.com/ as requested in the bug ticket.

I can see the global Gatsby theme nav was disabled on mobile views and set to be static rather than sticky. I caught and resolved a couple complications from removing the mobile display: none rule, but it's worth some manual exploration to confirm there aren't regressions I didn't clock.

Notes

1. My changes in 16f614f modify some styles that link back to changes in 637f7e0, which I'm not familiar with. Might be worth someone close to that work giving this MR's changes a glance

2. I added a z-index for the GlobalHeader that hopefully matches the repo’s convention (10, 100, 1000, etc.) I went with the highest I could find, but in looking it up, I'm not 100% convinced that .iframe-container class (where I grabbed the z-index: 1000000 value from) is being used. Still went with it to be better safe than sorry. Without the z-index a number of the homepage elements will display at a higher layer than the GlobalHeader.

3. While working on this, I noticed and raised an issue that should be addressed on the Gatsby theme repo: GlobalHeader is cut off by overflow:hidden on narrow viewports gatsby-theme-newrelic#79

[tyreer]

Hm, a bit stumped by the Jest check failing.

I pushed 7ff7b7f to update the snapshots, and npm run test passes all tests on my local branch.

[jerelmiller]

Hey @tyreer thanks for getting to this! Once we merge newrelic/gatsby-theme-newrelic#78, I'd love to have you update this PR to use the latest version of the gatsby theme which should also address the mobile view.

That position: static was a temporary adjustment on my part 😅 so glad to see you add a proper fix for its UX! As always, thanks so much for the contribution!

[tyreer]

Sounds like a plan @jerelmiller. Figured it might make sense to get the Gatsby fix in first, so thanks for hammering that out!

I'll keep an eye on newrelic/gatsby-theme-newrelic#78 and update this PR once the dependency update is published for @newrelic/gatsby-theme-newrelic

[jerelmiller]

@tyreer the PR has been merged and a v1.7.0 of the Gatsby theme has been published!

[tyreer]

@jerelmiller Pulled in the the new Gatsby theme and the GlobalHeader is looking great!

BUT, the Jest tests have been thrown off 😐

Getting the error below tracing back the GlobalHeader being rendered via TestRenderer.

Warning: An invalid container has been provided. This may indicate that another renderer is being used in addition to the test renderer. (For example, ReactDOM.createPortal inside of a ReactTestRenderer tree.) This is not supported.

Might be related to react-test-renderer attempting to go beyond a shallow render?

• This comes up pretty quick in troubleshooting: React-test-renderer: support for portal facebook/react#11565.
• And React team's response doesn't seem to point to a fix being implemented beyond the warning we're seeing if I'm following that issue correctly:

The test renderer was kind of intended to not be used with deep dependents.

I tried some of the ReactDOM.createPortal mocks mentioned in that issue, but it didn't get me far.

Have you encountered similar Jest errors before?

[jerelmiller]

Interesting. I think this error was obscured from us because there was some logic in versions < 1.7 that hid the overlay (which uses portals) in tests during render. The snapshot tests must not have been rendering the overlay until v1.7.

Apparently I have not done much testing using Portals in react-test-renderer because I was unaware this was an issue. Out of curiosity, what issues were you running into trying to address the error using the suggested fixes?

[jerelmiller]

@tyreer by chance are you seeing issues with a @keyframes syntax error?

If so, I think I spotted something odd in the gatsby theme that might be causing this issue that I'd like to fix.

[jerelmiller]

I've opened a PR in the Gatsby theme repo that should address the jest issue you are seeing. Turns out we were including styles from @elastic/search-ui that included keyframes, which emotion does not seem to like while trying to serialize styles in snapshot tests. You should be able to mock the Portal successfully after that PR gets merged and published 🤞

[tyreer]

Thanks for pointing out the changes on the Gatsby theme. Nice addition to the docs there. I'll consult once that PR goes through.

I think this error was obscured from us because there was some logic in versions < 1.7 that hid the overlay (which uses portals) in tests during render.

Yeah, seems sensible to me. I tried bringing in the new Gatsby theme into the developer-website repo just to see if its tets had a hard time as well, but it had no effect on those test runs, which look more unit-test focused than this repo's snapshot tests. Makes me think the snapshots here had just never been tasked with rendering that way

Out of curiosity, what issues were you running into trying to address the error using the suggested fixes?

They just didn't have any effect. Same local Jest run errors with or without them.

@Keyframes syntax error?

None to speak of. I'm getting the same errors locally as in the run on this PR https://github.com/newrelic/opensource-website/pull/608/checks?check_run_id=994942678

I'll keep an eye on newrelic/gatsby-theme-newrelic#81 and circle back once that's through. Cheers!

[jerelmiller]

v1.7.1 has just been published. 🤞 that PR will address the testing issues you have been experiencing on this branch.

I'm getting the same errors locally as in the run on this PR

I added a new section on Testing to the Gatsby theme. I believe the transformIgnorePatterns is already configured properly, but you should have better luck mocking the createPortal API to address the issues in the snapshot test.

I added a snapshot test for the global header in that PR to confirm that snapshot tests work correctly and they passed ok.

Let me know if you have other issues here! I'd love to get this one out. Thanks for being patient and helping us uncover other issues that needed addressing!

[CLAassistant]

All committers have signed the CLA.

[tyreer]

Hiya @jerelmiller 👋

I've pulled in v1.7.1 but haven't had any luck with the mocking.

I set up a __mocks__/react-dom.js file with the createPortal override as suggested here.

But I'm unclear how to apply that override in a given test. How would it be implemented in a spec.js file?

Alternately, I again tried following the approaches for these 2 suggestions on the React repo issue, but to no effect:

• React-test-renderer: support for portal facebook/react#11565 (comment)
• React-test-renderer: support for portal facebook/react#11565 (comment)

I wonder if the override makes it down to Overlay? Since @newrelic/gatsby-theme-newrelic has its own defined dependencies, I wonder if the react-dom from the Gatsby theme is unaffected by the mock override of createPortal. Totally speculating! I might just be mocking ineffectively. 🤷‍♂️

Thanks for working with me on this! Hope it isn't turning into more than you bargained for. 😄

[tyreer]

Another point I've noticed that I'm unsure how best to tackle: the Overlay has a z-index: 100, but this repo has a good number of elements above that layer.

As mentioned in the PR description, I've put GlobalHeader at z-index: 1000000 following the repo's convention. The .primary-header-logo is at 10000. Both of which overlap the Overlay when you click search:

In all, I'm seeing 17 rules with z-index at or above 100. I suppose they all could be lowered down bellow 100, but that's a bit of a task. Setting Overlay to a level higher than any of those in this codebase could feels peak z-index wars. But perhaps this isn't the only codebase where the issue will come up?

Any sense of a preferred approach to z-index fun between the two repos?

[jerelmiller]

Any sense of a preferred approach to z-index fun between the two repos?

@danielgolden @djsauble This is a really good question from @tyreer. Do we have a preferred approach to z-indexes? I think we just kinda picked one out-of-the-hat in the theme, but clearly we've got issues with other elements overlapping. Could you provide some feedback here?

[jerelmiller]

I wonder if the override makes it down to Overlay?

I thought so since jest should use the mock whenever import ... from 'react-dom' is used.

But I'm unclear how to apply that override in a given test. How would it be implemented in a spec.js file?

You shouldn't need to do anything special in the spec file. The __mocks__ convention uses a special feature of jest (manual mocks) that allow you to mock entire dependencies. This means any import from react-dom should use the mock instead of the actual dependency implemenetation.

I might try the upgrade + some different ideas in a separate branch to get a better feel. Perhaps the test in the theme repo was a misnomer since its mocking react-dom in the repo itself and maybe that gave me a false sense of hope 😅 . The jest mocking is super cool, but I've always been a bit hazy on details for when it kicks in and when it doesn't, especially for dependencies of dependencies in node_modules.

Thanks so much for being patient! We'll get this one way or another 💪

[djsauble]

@tyreer @jerelmiller Good catch on the z-index! The overlay should trump both the logo and global header, but you're right that we don't want an escalating z-index war. I suggest we cap the z-index scale to 1000, hopefully that's high enough to fix the overlay issue without resorting to a major refactor though we may want to do that eventually for consistency.

[jerelmiller]

@tyreer I opened a draft PR to try and isolate the tests to ensure there wasn't anything else at play. From that branch, it looks like I'm able to get the tests run successfully.

Can you try out the changes in that PR (just re-implement them here) and see if that works? If not, I'd love it if you could paste the output from one of the failures to see if we spot anything else at play.

[jerelmiller]

I notice a lot of these snapshots have these conflict indicators in here. I'm wondering if this is part of the problem. After adding the react-dom mock, could you run npm test -- -u to re-update these and see if tests pass for you?

[tyreer]

Nailed it!

Not sure how those merge conflict came up. Seemed odd to have them in snapshots of all places. I had run the alias test:update, but maybe the mock file wasn't properly in place at the time and I forgot to run another snapshot update after I'd put the mock in.

Clearly missed loads when trying to resolve in e2daec8, but that kinda pointed us in the right direction in the end.

Sorted now! 🎉 Thanks a lot for stepping through this with me.

[jerelmiller]

🎉 🎉 🎉 🎉 🎉

So glad that worked! Glad I could help!

[tyreer]

I suggest we cap the z-index scale to 1000

Sounds sensible to me @djsauble

So we'd say the Overlay in the Gatsby theme would change to have z-index: 1000, correct?

This repo has 6 layers. Any preference between the two options below for a refactor? I think option B is cleaner, personally.

Option A

Current z-index	New z-index
1	1
10	10
100	100
1000	200
10000	300
100000	400

Option B

Current z-index	New z-index
1	100
10	200
100	300
1000	400
10000	500
100000	600

[djsauble]

@tyreer I'm a sucker for linear scales, Option B looks great. 🙂

[tyreer]

Right on. I think c4145ac does the trick @djsauble.

We just need the Gatsby theme to publish Overlay at z-index: 1000 and we should be good to go. Adjusting Overlay to 1000 locally in DevTools looks real nice.

[jerelmiller]

I can get a quick PR to adjust the overlay z-index. In the mean time, I see no reason not to accept this PR. Thanks so much for working through this!

[nr-opensource-bot]

🎉 This PR is included in version 1.8.5 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

[danielgolden]

Thanks for all the time and effort you put into this PR, @tyreer!! We really appreciate it :)

[tyreer]

You're welcome @danielgolden ! Glad we were able to sort out the curve balls and happy I could help out 😃