Page MenuHomePhabricator

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

Authored by ginsu on May 16 2023, 5:29 AM.
Tags
None
Referenced Files
F3408885: D7835.id26531.diff
Wed, Dec 4, 2:18 PM
F3407411: D7835.diff
Wed, Dec 4, 6:04 AM
Unknown Object (File)
Sun, Nov 17, 2:18 AM
Unknown Object (File)
Sun, Nov 17, 1:40 AM
Unknown Object (File)
Sun, Nov 17, 1:40 AM
Unknown Object (File)
Sun, Nov 17, 1:40 AM
Unknown Object (File)
Sun, Nov 17, 1:40 AM
Unknown Object (File)
Sat, Nov 16, 11:28 PM

Details

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

Event Timeline

ginsu requested review of this revision.May 16 2023, 5:49 AM

remove unnecessary classes

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

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
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