Page MenuHomePhabricator
Differential D7868

[landing] implement on click functions in header
ClosedPublic
Actions

Authored by ginsu on May 19 2023, 1:51 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Mar 27, 11:54 PM
Unknown Object (File)
Mar 26 2024, 11:38 PM
Unknown Object (File)
Feb 27 2024, 7:06 PM
Unknown Object (File)
Feb 26 2024, 5:53 PM
Unknown Object (File)
Feb 26 2024, 5:53 PM
Unknown Object (File)
Feb 26 2024, 5:53 PM
Unknown Object (File)
Feb 26 2024, 5:53 PM
Unknown Object (File)
Feb 26 2024, 5:53 PM
View All 32 Files
Subscribers
ashoat
atul
tomek

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
Lint Not Applicable
Unit
Tests Not Applicable

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

ginsu updated this revision to Diff 26723.May 21 2023, 1:08 AM

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.

ginsu added inline comments.May 22 2023, 6:06 AM
landing/header.css
12 ↗(On Diff #26665)

great idea, will update the diff accordingly

ginsu updated this revision to Diff 26771.May 22 2023, 6:17 AM

create z-index css variables

Harbormaster completed remote builds in B19586: Diff 26771.May 22 2023, 6:35 AM
ginsu updated this revision to Diff 29645.Aug 8 2023, 12:06 PM

rebase before landing

Harbormaster completed remote builds in B21605: Diff 29645.Aug 8 2023, 12:28 PM
Closed by commit rCOMMd09603797395: [landing] implement on click functions in header (authored by ginsu). · Explain WhyAug 8 2023, 2:07 PM
This revision was automatically updated to reflect the committed changes.
ginsu added a commit: rCOMMd09603797395: [landing] implement on click functions in header.

Revision Contents
Changeset List

PathSize
landing/
3 lines
9 lines
32 lines
2 lines
17 lines
2 lines

Diff 29720

View Options

landing/global.css

Loading...
View Options

landing/header.css

Loading...
View Options

landing/header.react.js

Loading...
View Options

landing/landing.css

Loading...
View Options

landing/landing.react.js

Loading...
View Options

landing/mobile-nav.css

Loading...