Details

Reviewers

atul
kamil

Commits

rCOMMd09603797395: [landing] implement on click functions in header

Summary

This diff is the third part to updating the Header component to match the redesigns. The focus of this part is to implement the onClick/tap functions to open and close the mobile nav header. The next diff will address the last part of updating the Header which will be to either show an "X" icon or hamburger menu icon in the header depending on if the mobile nav header is active or not

Depends on D7863

Test Plan

Please watch the demo videos to see how the Header looks and behaves:

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

ginsu created this revision.May 19 2023, 1:51 AM

Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptMay 19 2023, 1:51 AM

ginsu edited the summary of this revision. (Show Details)May 19 2023, 1:56 AM

ginsu edited the test plan for this revision. (Show Details)

ginsu added reviewers: atul, kamil.

ginsu added a parent revision: D7863: [landing] clean up header icons user experience.

ginsu attached a referenced file: F545237: Screen Recording 2023-05-19 at 11.55.35 AM.mov. (Show Details)

Harbormaster completed remote builds in B19500: Diff 26665.May 19 2023, 2:09 AM

ginsu requested review of this revision.May 19 2023, 2:09 AM

ginsu added a child revision: D7869: [landing] show close icon when mobile nav is active.May 19 2023, 4:50 AM

atul accepted this revision.May 19 2023, 10:42 AM

atul added inline comments.

landing/header.css
12 ↗	(On Diff #26665)	I think we attempt to keep z-index somewhat reasonable relative to other z-indexes. I worry that next time someone might just try `99999` or `999999` etc. which defeats the purpose of setting this one extremely high? But on the other hand this seems to be the only usage of `z-index` in `landing` so maybe it's fine? Defer to you
landing/landing.react.js
88–93 ↗	(On Diff #26665)	Might make sense to do something like let header; if (!onQR) { header = (...); } Up to you though, looks like you're matching the pattern used for footer below

This revision is now accepted and ready to land.May 19 2023, 10:42 AM

ginsu added inline comments.May 21 2023, 12:59 AM

landing/header.css
12 ↗	(On Diff #26665)	Ugh I didn't hit submit on my initial inline comment, sorry about that, but what I was going to write is that we always want the header to be top most element in the `z-index` so I chose 999 as an arbitrary high number to make sure if we ever added other elements with a `z-index` the header will still be the top. I worry that next time someone might just try 99999 or 999999 etc. which defeats the purpose of setting this one extremely high? You brought up a good point here so I will make the `z-index` more reasonable. But on the other hand this seems to be the only usage of z-index in landing so maybe it's fine? In `mobile-nav.css` there is a `mobileNav` css class with a `z-index` of 1 and in `landing.css` there is a `container` css class with a `z-index` of 0
landing/landing.react.js
88–93 ↗	(On Diff #26665)	I'll update this

address comments

Harbormaster completed remote builds in B19542: Diff 26723.May 21 2023, 1:27 AM

tomek added inline comments.May 22 2023, 12:50 AM

landing/header.css
12 ↗	(On Diff #26665)	What do you think about defining a css variables in one place for all the z-index values? Then we can give them meaningful names, e.g. `header-z-index` and be able to move the layers consciously.