Page MenuHomePhabricator

[landing] introduce competitor feature card component
ClosedPublic

Authored by ginsu on May 22 2023, 6:19 AM.
Tags
None
Referenced Files
F1430809: D7905.id.diff
Wed, Mar 27, 11:53 PM
Unknown Object (File)
Wed, Mar 6, 9:49 AM
Unknown Object (File)
Mon, Mar 4, 12:37 AM
Unknown Object (File)
Mon, Mar 4, 12:37 AM
Unknown Object (File)
Feb 10 2024, 4:59 PM
Unknown Object (File)
Feb 10 2024, 4:58 PM
Unknown Object (File)
Feb 10 2024, 4:58 PM
Unknown Object (File)
Feb 10 2024, 4:58 PM
Subscribers

Details

Summary

This diff introduces the CompetitorFeatureCard that will be used in the competitor comparison section of the landing page.

Here are what the cards look like in the Figma:

Screenshot 2023-05-22 at 4.23.23 PM.png (2×3 px, 1 MB)

Depends on D8016

Test Plan

Please see the following screenshots/demos below:

Please not the focus on this diff is just the cards itself, the container for the cards will be cleaned up in a subsequent diff

Desktop/Bigger device view:

Screenshot 2023-05-22 at 2.44.57 PM.png (2×3 px, 909 KB)

Hover effect:

Mobile view:

Screenshot 2023-05-22 at 3.49.52 PM.png (2×3 px, 1 MB)

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

landing/competitor-feature-card.css
25 ↗(On Diff #26772)

Saw @atul do this in D7764 with the hr border-top style, so I think having this should be fine too

landing/global.css
43 ↗(On Diff #26772)

This was a color used in the Figma but was missing in the design system. I did update the design system in the Figma so everything is still in sync

ginsu requested review of this revision.May 22 2023, 6:37 AM
atul requested changes to this revision.May 22 2023, 9:03 AM

Let's definitely fix the typos and try to color icon via CSS.

landing/competitor-feature-card.css
35 ↗(On Diff #26772)

typo here

landing/competitor-feature-card.react.js
14 ↗(On Diff #26772)

Would it make sense to include vectors of competitor logos in repo similar to background-illustration.react.js so they're included in bundle instead of having to fetch asset from network?

Based on super quick google search SVGs of logos are pretty accessible:
https://github.com/signalapp/Signal-Desktop/blob/87d6a55ce87a3f09e34da1dbc9e69f400ac7e32d/images/signal-logo.svg

4b8c68.png (1×1 px, 400 KB)

2ef8ce.png (492×842 px, 44 KB)

etc

33 ↗(On Diff #26772)

typo here

51 ↗(On Diff #26772)

Can we color via CSS instead so we can use color names from design system instead of hardcoding color prop?

You can look at expand-buttons.react.js as an example.

73 ↗(On Diff #26772)

Should we have a separate name here since this isn't a "competitor logo"?

Alternatively, we could rename from competitor logo, to service logo or something along those lines?

This revision now requires changes to proceed.May 22 2023, 9:03 AM
ginsu edited the test plan for this revision. (Show Details)

address comments

landing/global.css
43–44

This was a color used in the Figma but was missing in the design system. I did update the design system in the Figma so everything is still in sync

atul added inline comments.
landing/competitor-feature-card.css
2

Personal preference would be if outermost container defined the max-width for each breakpoint and everything within was laid out w/ flexbox to fill the space as intended.

25

Should this be defined in "the design system"?

landing/competitor-feature-card.react.js
65–78

Maybe not necessary if we're only using these here. But what would you think about a lightweight

<CompetititorLogo name="telegram" size={30} />

sort of wrapper component to handle this branching?

83

So this is an h1 with typography.heading3 and css.headingText...

This revision is now accepted and ready to land.May 24 2023, 9:25 AM
ginsu added inline comments.
landing/competitor-feature-card.react.js
65–78

D8015 address this and I will update this diff to use these new CompetititorLogo components

landing/competitor-feature-card.css
2

A subsequent diff will improve on this

25

Saw you do this in D7764 with the hr border-top style, so I thought having it this way fine, it's also a color hex with an opacity attached with it; however, I guess it wouldn't hurt so I will update the diff accordingly

address comments

landing/global.css
43–45 ↗(On Diff #27232)

These were colors used in the Figma but was missing in the design system. I did update the design system in the Figma so everything is still in sync