Page MenuHomePhabricator
Differential D7835

[landing] update header strucutre to use flexbox over grid
ClosedPublic
Actions

Authored by ginsu on May 16 2023, 5:29 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Apr 2, 3:59 AM
Unknown Object (File)
Tue, Apr 1, 5:47 PM
Unknown Object (File)
Tue, Apr 1, 4:45 PM
Unknown Object (File)
Tue, Apr 1, 7:34 AM
Unknown Object (File)
Tue, Apr 1, 2:43 AM
Unknown Object (File)
Mon, Mar 31, 4:27 PM
Unknown Object (File)
Sat, Mar 8, 6:44 PM
Unknown Object (File)
Sat, Mar 8, 6:44 PM
View All Files
Subscribers
ashoat
atul
• ted
tomek

Details

Reviewers
atul
kamil
Commits
rCOMMafe2d5a38a0b: [landing] update header strucutre to use flexbox over grid
Summary

To update the Header component to match the redesigns, I broke it up into three diffs. This is the first diff. This diff removes the grid layout in favor for a flexbox layout. I chose to do this for several reasons:

  1. Flexbox is a bit more simple to use/understand
  2. In the previous landing page header we had two rows when resizing; however, with the introduction of a MobileNav component we now are only going to use one row so having a grid is actually unnecessary.
  3. As long as we set the breakpoints correctly, Flexbox will work just fine

For context here is what the two row header looked like, and would be shown when using a smaller device:

Screenshot 2023-05-16 at 2.40.14 PM.png (420×726 px, 200 KB)

In addition to removing the grid layout in the header, this diff adds the hamburger menu icon that will be displayed whenever we have a width less than 480px

Depends on D7781

Test Plan

The header looks and behaves as expected when resizing the window to different widths. Please note the demo might not be super polished, but the very next diff in the stack will address this. The next diff adds new elements so I felt that it was unnecessary to polish something that would get changed very quickly

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ginsu created this revision.May 16 2023, 5:29 AM
Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptMay 16 2023, 5:29 AM
ginsu edited the summary of this revision. (Show Details)May 16 2023, 5:43 AM
ginsu edited the test plan for this revision. (Show Details)
ginsu added reviewers: atul, kamil.
ginsu added a parent revision: D7781: [landing] animate mobile nav component.
ginsu attached a referenced file: F540836: Screen Recording 2023-05-16 at 2.41.54 PM.mov. (Show Details)
ginsu attached a referenced file: F540833: Screenshot 2023-05-16 at 2.40.14 PM.png. (Show Details)
Harbormaster completed remote builds in B19389: Diff 26527.May 16 2023, 5:49 AM
ginsu requested review of this revision.May 16 2023, 5:49 AM
ginsu updated this revision to Diff 26531.May 16 2023, 5:51 AM

remove unnecessary classes

ginsu added a subscriber: • ted.May 16 2023, 5:59 AM
ginsu added inline comments.
landing/header.css
37 ↗(On Diff #26531)

For some reason this css file used snake case naming convention while all the other css files used camel case. I thought this was strange, and updated the naming convention in this diff to be consistent with the rest of the css files

104–112 ↗(On Diff #26531)

Spoke to @ted about this in our sync, but we decided not to add a hover effect similar to what the other social icons have for the hamburger menu icon for several reasons:

  1. It was not included in the figma designs
  2. Since we aren't redirecting the user to a different site, there is a weird behavior where the hover effect does not immediately go away on mobile devices

If we want to revisit this in the future, I will create a follow up diff; however, I think we should consider this outside the scope of this particular diff

Harbormaster completed remote builds in B19393: Diff 26531.May 16 2023, 6:09 AM
ginsu added a child revision: D7839: [landing] introduce new header elements to header component.May 16 2023, 6:18 AM
atul accepted this revision.May 16 2023, 8:01 AM

Flexbox is a bit more simple to use/understand

FWIW I think CSS Grid is pretty intuitive and works naturally w/ Flexbox. Found https://learncssgrid.com helpful in understanding how it works. That said, the way it has been used in parts of landing are maybe not the most idiomatic.

I think it makes sense to remove Grid here for the other reasons stated, but I still think it's a "tool we should keep in the toolbox."

landing/header.css
11 ↗(On Diff #26531)

Do we need to specify hardcoded width here?

104–112 ↗(On Diff #26531)

It was not included in the figma designs

Hover states are often missing from Figma designs, not sure if that's a reason to exclude them. I think we should match the hover effects we have throughout landing.

Ben previously removed hover states at some point, and then we ended up adding them back. Might be easiest to just leave them in.

This revision is now accepted and ready to land.May 16 2023, 8:01 AM
ginsu mentioned this in D7863: [landing] clean up header icons user experience.May 18 2023, 10:11 AM
ginsu added inline comments.May 18 2023, 10:15 AM
landing/header.css
11 ↗(On Diff #26531)

Adding the width field here prevent this from happening:

Screenshot 2023-05-18 at 8.13.25 PM.png (2×3 px, 856 KB)

104–112 ↗(On Diff #26531)

Created D7863 to address this

ginsu attached a referenced file: F544515: Screenshot 2023-05-18 at 8.13.25 PM.png. (Show Details)May 18 2023, 10:15 AM
ginsu updated this revision to Diff 29642.Aug 8 2023, 12:02 PM

rebase before landing

Harbormaster completed remote builds in B21602: Diff 29642.Aug 8 2023, 12:22 PM
Closed by commit rCOMMafe2d5a38a0b: [landing] update header strucutre to use flexbox over grid (authored by ginsu). · Explain WhyAug 8 2023, 2:07 PM
This revision was automatically updated to reflect the committed changes.
ginsu added a commit: rCOMMafe2d5a38a0b: [landing] update header strucutre to use flexbox over grid.
ginsu mentioned this in rCOMM9f3ce033ed46: [landing] clean up header icons user experience.

Revision Contents
Changeset List

PathSize
landing/
106 lines
59 lines

Diff 29642

View Options

landing/header.css

Show First 20 Lines • Show All 130 Lines • ▼ Show 20 Lines
const styles = StyleSheet.create({ const styles = StyleSheet.create({
dummyMessage: { dummyMessage: {
paddingHorizontal: 12, paddingHorizontal: 12,
paddingVertical: 6, paddingVertical: 6,
}, },
message: { message: {
overflow: 'hidden', overflow: 'hidden',
paddingHorizontal: 12, paddingHorizontal: 12,
paddingVertical: 6, paddingVertical: 8,
}, },
text: { text: {
fontFamily: 'Arial', fontFamily: 'Arial',
fontSize: 18, fontSize: 14,
benschacAuthorUnsubmitted
Done
Inline Actions

I actually think this is a bit too small. But that's what's in the design.

https://www.figma.com/file/L675ETKDnGaSwlpZAw4MIC/Mobile-App?node-id=351%3A455

@ashoat - I think 16px looks better.

image.png (1×2 px, 2 MB)

benschac: I actually think this is a bit too small. But that's what's in the design. https://www.figma.
lineHeight: 22,
}, },
}); });
export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement }; export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement };
View Options

landing/header.react.js

Show First 20 Lines • Show All 130 Lines • ▼ Show 20 Lines
const styles = StyleSheet.create({ const styles = StyleSheet.create({
dummyMessage: { dummyMessage: {
paddingHorizontal: 12, paddingHorizontal: 12,
paddingVertical: 6, paddingVertical: 6,
}, },
message: { message: {
overflow: 'hidden', overflow: 'hidden',
paddingHorizontal: 12, paddingHorizontal: 12,
paddingVertical: 6, paddingVertical: 8,
}, },
text: { text: {
fontFamily: 'Arial', fontFamily: 'Arial',
fontSize: 18, fontSize: 14,
benschacAuthorUnsubmitted
Done
Inline Actions

I actually think this is a bit too small. But that's what's in the design.

https://www.figma.com/file/L675ETKDnGaSwlpZAw4MIC/Mobile-App?node-id=351%3A455

@ashoat - I think 16px looks better.

image.png (1×2 px, 2 MB)

benschac: I actually think this is a bit too small. But that's what's in the design. https://www.figma.
lineHeight: 22,
}, },
}); });
export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement }; export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement };