Details

Reviewers

atul
kamil
ashoat

Commits

rCOMM85fa15d3496a: [landing] animate mobile nav component

Summary

This diff animates the mobile nav component from the top to the bottom. I worked closely with Ted on this to get the animation exactly how he wanted it to look/feel. Some things to note is that this diff is safe to land in its current state. The mobile nav component is currently hidden above the screen where it lives until someone presses the hamburger icon which will be introduced in subsequent diffs. Furthermore the first demo in the test plan may look a bit funky and that is because we need to make changes to the Header component as well. I will attach a second demo which will preview the "final form".

The focus of this diff is purely the up and down animation the mobile nav bar does

Depends on D7763

Test Plan

Please watch the demo videos below to see how the up and down animation of the mobile nav bar looks and feels

Sneak Peak of the animation with a more polished header:

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ginsu created this revision.May 10 2023, 6:17 AM

Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptMay 10 2023, 6:17 AM

update

Harbormaster completed remote builds in B19240: Diff 26342.May 10 2023, 6:35 AM

fix memo

Harbormaster completed remote builds in B19242: Diff 26344.May 10 2023, 6:49 AM

ginsu added inline comments.May 10 2023, 6:57 AM

landing/landing.css
22 ↗	(On Diff #26345)	This is to prevent scrolling when the `MobileNav` component is active
26 ↗	(On Diff #26345)	This is to prevent the `MobileNav` from covering the `Header`
landing/landing.react.js
88–96 ↗	(On Diff #26345)	We should be memoizing here in order to memoize the props getting passed down to `MobileNav`
88–96 ↗	(On Diff #26345)	We also don't need to worry about gating `MobileNav` based on `onQR` since `MobileNav` is only accessible from `Header` which is already gated based on `onQR`

ginsu edited the summary of this revision. (Show Details)May 10 2023, 6:59 AM

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

ginsu added reviewers: atul, kamil.

ginsu added a parent revision: D7763: [landing] introduce mobile nav compoment.

ginsu attached a referenced file: F533185: Screen Recording 2023-05-09 at 6.06.27 PM.mov. (Show Details)

ginsu attached a referenced file: F533184: Screen Recording 2023-05-10 at 2.27.58 PM.mov. (Show Details)

Harbormaster completed remote builds in B19243: Diff 26345.May 10 2023, 7:04 AM

ginsu requested review of this revision.May 10 2023, 7:04 AM

ginsu mentioned this in D7766: [landing] memoize header and footer.May 10 2023, 7:09 AM

ashoat requested changes to this revision.May 12 2023, 1:59 AM

ashoat added inline comments.

landing/landing.react.js
88–96 ↗	(On Diff #26345)	We should be memoizing here in order to memoize the props getting passed down to `MobileNav` This doesn't make any sense. You remain deeply confused about memoization

This revision now requires changes to proceed.May 12 2023, 1:59 AM

remove memoization around mobile nav component

ginsu added inline comments.May 15 2023, 5:47 AM

landing/landing.css
22 ↗	(On Diff #26462)	This is to prevent scrolling when the MobileNav component is active
26 ↗	(On Diff #26462)	This is to prevent the MobileNav from covering the Header
landing/landing.react.js
103–106 ↗	(On Diff #26462)	We don't need to worry about gating `MobileNav` based on `onQR` since `MobileNav` is only accessible from `Header` which is already gated based on `onQR`

Harbormaster completed remote builds in B19337: Diff 26462.May 15 2023, 6:03 AM

Memoization concern has been addressed; will leave the rest of the review to the original selected reviewers

landing/mobile-nav.react.js
14–17 ↗	(On Diff #26462)	All React props should always be read-only

atul accepted this revision.May 15 2023, 12:58 PM

atul added inline comments.

landing/mobile-nav.css
11 ↗	(On Diff #26462)	We typically use `ms` units for `transition` so this would be `500ms` instead of `0.5s`. That said, not sure if it's something we care about being consistent w/ so defer to you.