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

Layout refactor using sticky [Closes #1848, Closes #1867] #1973

Merged
merged 7 commits into from
Nov 30, 2020
Merged

Layout refactor using sticky [Closes #1848, Closes #1867] #1973

merged 7 commits into from
Nov 30, 2020

Conversation

wackerow
Copy link
Member

@wackerow wackerow commented Nov 29, 2020

Description

This refactor changes the top Nav component (which includes EDN navigation) and SideNavMobile components to be positioned sticky instead of fixed. This should fix issues with banners not being aligned properly, as they were not positioned relative to their fixed siblings.

  • Nav/index.js and SideNavMobile.js both changed to position: sticky
  • Moved <SideNavMobile> outside of <MainContainer>
  • Removed conditional margin-top styling to MainContainer, as this will now sit relative to nav components naturally
  • Removed now unused shouldShowSubNav constant (and its dependencies)

Also, while in the Layout.js file, there are two deprecated methods being used (.addListener(() => {}) and .removeListener(() => {})... these are now .addEventListener("change", () => {}) and .removeEventListener("change", () => {})). I was going to update these, however, dark mode state is being loaded in useEffect already, and changed via handleThemeChange() directly which re-renders the view. I removed these calls, and the dark mode seems to work perfectly fine without it.

⚠️ Given that this is a layout adjustment that potentially influences every single page, any help testing this would be appreciated. Locally it is working exactly like the live site on dev pages, EDN, ETH2, and all the MDX pages (except with no banner issues on reloads or direct linking).
Please let me know if you spot something I missed... (Note, this issue has been challenging to reproduce in dev, at least for me, but this should hopefully fix the banner issue, will test the Netlify link when this saves)

Related Issue #1848

depracated format, and state handled elsewhere
Changed `Nav/index.js` and `SideNavMobile.js` to use `sticky` positioning, instead of `fixed`, to fix alignment glitches affecting banners
@wackerow wackerow changed the title Layout refactor using sticky [Fixes #1848] Layout refactor using sticky [Closes #1848, Closes #1867] Nov 30, 2020
@wackerow wackerow marked this pull request as draft November 30, 2020 02:19
Fixes issue with SideNavMobile not displaying name next to icon. Name was inadvertantly being wrapped in a disabled <BannerNotification> tag.
@wackerow
Copy link
Member Author

So this wasn't intended to address/involve issue #1866, but that issue now appears to be affecting this Netlify build. This is getting very confusing for forgive me if I post some screenshots to demonstrate and also think through what is going on here.

Example of issue, using /en/developers/docs

This PR branch: Netlify vs Localhost

This is a comparison of the netlify build for this PR (on the left) and the same page running on localhost (on the right):
image
Notice that highlighted elements should match, but don't. In this case, the Netlify version has wrapped the SideNavMobile element inside a MainContainer element, which completely changes the way SideNavMobile is styled and renders (and it may not render at all depending on that styling).

ethereum.org vs localhost dev-branch

Similarly, this compares the live page on the left to my local dev on the right:
image
Layout_MainContent is, similar to above, wrapping the intended element SideNavMobile when it shouldn't, causing this:
image
This giant space is actually the SideNavMobile component completely re-styled after being inadvertently wrapped in a BannerNotification which is wrapped in MainContent. You can see the term "README" nested inside this giant white space:
image

Lastly, Netlify of this PR: From homepage vs reload on /docs/ page

Navigating to docs from homepage:
image
After refresh:
image

@samajammin @ryancreatescopy Sorry for all the screenshots, but testing this has been a challenge and needed to try and organize what's happening here. Have either of you seen anything like this before where a component for some reason wraps its sibling? Especially with these discrepancies between environments, browsers, devices, and how you got to the page? Let me know if you have any thoughts, but switched this to draft-mode in the meantime.

On the plus side, the initial changes this proposed seem to be working nicely to fix the top alignment issues we've been having (when this other bug isn't getting in the way causing side-effects).

@samajammin
Copy link
Contributor

Hey @wackerow - thanks for this PR! Appreciate the detailed comment as well.

I have indeed seen the issue you mention. I'm not yet sure what's going on but I will investigate. I suspect something strange is happening with our CDN caching.

Out of curiosity, does clearing your browser cache appear to resolve this for the preview deploy / production deploy?

@wackerow
Copy link
Member Author

@samajammin Yes, actually... as I was checking that I noticed similarly it worked in a private browser window, and also seemed to have an instance that corrected after toggling light/dark mode. This actually fixed all of it for me (at least in the meantime) for both netlify builds and the live site.

@samajammin samajammin marked this pull request as ready for review November 30, 2020 18:44
Copy link
Contributor

@samajammin samajammin left a comment

Choose a reason for hiding this comment

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

This looks great! Nice simplification of the layout. Thanks for doing this ❤️

src/components/Layout.js Show resolved Hide resolved
@samajammin
Copy link
Contributor

Great work! I'm comfortable merging this as I'm confident these changes are unrelated to #1866.

@samajammin samajammin merged commit 69eb10a into ethereum:dev Nov 30, 2020
@wackerow wackerow deleted the w/layout#1848 branch November 30, 2020 19:04
@samajammin samajammin mentioned this pull request Dec 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants