Page MenuHomePhabricator

[landing] [feat] [ENG-353] add team layout
ClosedPublic

Authored by benschac on Apr 7 2022, 11:42 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 13, 12:47 PM
Unknown Object (File)
Wed, Nov 13, 12:47 PM
Unknown Object (File)
Wed, Nov 13, 12:47 PM
Unknown Object (File)
Wed, Nov 13, 12:47 PM
Unknown Object (File)
Wed, Nov 13, 12:47 PM
Unknown Object (File)
Wed, Nov 13, 12:47 PM
Unknown Object (File)
Tue, Nov 12, 2:00 AM
Unknown Object (File)
Sun, Nov 10, 12:51 AM

Details

Summary

adds generic layout to match expo.dev https://expo.dev/about

Test Plan

layout should generally look the same

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

benschac added a parent revision: Restricted Differential Revision.Apr 7 2022, 11:43 AM
atul requested changes to this revision.Apr 8 2022, 10:30 AM

Added some questions inline, also adding a screenshot would be helpful

landing/team.css
11–27 ↗(On Diff #11201)

There aren't any designs from Figma to go off of here, but assuming you tried viewing the site at different breakpoints and made sure it looks good/readable.

48 ↗(On Diff #11201)

Is there a reason we're introducing a new unit here?

Equal to the used advance measure of the “0” (ZERO, U+0030) glyph found in the font used to render it. (The advance measure of a glyph is its advance width or height, whichever is in the inline axis of the element.)

So I'm guessing you want it to be roughly 60-characters wide?

48 ↗(On Diff #11201)

It looks like we're introducing a new unit (ch) into the codebase here..

Equal to the used advance measure of the “0” (ZERO, U+0030) glyph found in the font used to render it. (The advance measure of a glyph is its advance width or height, whichever is in the inline axis of the element.)

Guessing you wanted to keep things to roughly 60 characters wide?

landing/team.react.js
11–16 ↗(On Diff #11201)

Does this open in a new window or new tab?

It doesn't look like we use window.open() elsewhere in the codebase?

Why can't we go with <a href=blah?

11–16 ↗(On Diff #11201)

I think this should be in a React.useCallback()

This revision now requires changes to proceed.Apr 8 2022, 10:30 AM
landing/team.css
11–27 ↗(On Diff #11201)

I did.

48 ↗(On Diff #11201)
landing/team.react.js
11–16 ↗(On Diff #11201)

I wanted to use the button component, which requires an onClick method.

I do think all buttons should have a required onClick because buttons are meant to be clicked.

I could make a <a> link style that is similar to the button element if you think that's a better approach. I think this is fine though.

Patched and took a screenshot

e6df.png (2×3 px, 7 MB)

Personally think things would be more symmetrical if "Team" was right above our profile pictures, as it is for "Team at Software Mansion" and the open roles section was moved above

landing/team.react.js
12–15 ↗(On Diff #11257)

Personally don't like that this opens the page in a new tab/window. I like deciding that myself by holding CMD

28 ↗(On Diff #11257)

Maybe onRolesClick?

This revision is now accepted and ready to land.Apr 11 2022, 10:59 AM
This revision now requires review to proceed.Apr 11 2022, 10:59 AM

@ashoat -- you're still a blocking review. Could you give this a look?

benschac retitled this revision from [landing] add team layout to [landing] [feat] [ENG-353] add team layout.Apr 11 2022, 1:41 PM
This revision is now accepted and ready to land.Apr 12 2022, 11:23 AM