[native] Introduce `CommIcon` component
ClosedPublic
Actions

Authored by atul on Aug 15 2022, 6:43 AM.

Details

Reviewers

• abosh
• jacek
tomek
varun

Commits

rCOMM45dfbce7acb5: [native] Introduce `CommIcon` component

Summary

Create a separate CommIcon pack that includes custom icons that aren't in the SWMansion icon pack.

Here are the included icons:

Screen Shot 2022-08-15 at 10.06.13 AM.png (808×868 px, 63 KB)

Test Plan

Will be tested in subsequent diffs that "consume" the CommIcon component.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

atul created this revision.Aug 15 2022, 6:43 AM

Herald added subscribers: • adrian, ashoat. · View Herald TranscriptAug 15 2022, 6:43 AM

atul added inline comments.Aug 15 2022, 6:46 AM

native/components/comm-icon.react.js
7–11 ↗	(On Diff #15612)	Basically copy/pasted from `native/components/swmansion-icon.react.js`

Harbormaster completed remote builds in B11347: Diff 15612.Aug 15 2022, 6:53 AM

atul requested review of this revision.Aug 15 2022, 6:53 AM

atul edited the summary of this revision. (Show Details)Aug 15 2022, 7:06 AM

Include additional required changes

Harbormaster completed remote builds in B11354: Diff 15619.Aug 15 2022, 12:37 PM

Some questions before I can accept:

How were you able to verify these icons didn't exist in the pack? The SWMansion icon pack has a reply-filled, cloud-filled, and sidebar-filled that are actively used in the codebase. I haven't seen the "subsequent diffs that "consume" the CommIcon component" but I'm assuming given that this diff exists we want to use these icons over the SWMansion icon pack ones going forward? Or is that interpretation wrong?
What's the difference between icons from both packs? You said that the SWMansion icon pack didn't contain these icons, but I found instances of reply-filled, cloud-filled, and sidebar-filled in the SWMansion icon pack, so if the icons are different, showing photos with their differences would be helpful. Otherwise, if we already have reply-filled, cloud-filled, and sidebar-filled icons, why are we switching away?
Minor, but comparing this diff to the current structure of the SWMansion icon pack in the codebase, there are some differences. For example, you wrote CommIcons.ttf whereas currently it's swmansion.ttf (all lowercase). Personally, I think CommIcons.ttf is better, especially looking at the rest of the Info.release.plist file which contains useful capitalization, but maybe the swmansion.ttf can be renamed to SWMansion.ttf or something. Or even include the word icons in it, like with CommIcons.ttf.

This revision now requires changes to proceed.Aug 15 2022, 12:51 PM

How were you able to verify these icons didn't exist in the pack? The SWMansion icon pack has a reply-filled, cloud-filled, and sidebar-filled that are actively used in the codebase. I haven't seen the "subsequent diffs that "consume" the CommIcon component" but I'm assuming given that this diff exists we want to use these icons over the SWMansion icon pack ones going forward? Or is that interpretation wrong?

Some context:

SWMansion released a free Icon Pack: https://dribbble.com/shots/15126704-Freebie-SWM-Icon-Pack
We used those SWMansion icons in our app
We ran into cases where we required a custom icon to meet some need that the SWMansion icon pack didn't handle
In those cases Alicia (and maybe Daniel?) designed custom icons specifically for Comm to be used in our app (eg cloud-filled)
Rather than separating out the two icon packs, we added the custom icons to the existing SWMansion icon pack
Now we're breaking them apart so the SWMansion icon pack can be "frozen," and we'll only add to the CommIcon pack in the future

What's the difference between icons from both packs? You said that the SWMansion icon pack didn't contain these icons, but I found instances of reply-filled, cloud-filled, and sidebar-filled in the SWMansion icon pack, so if the icons are different, showing photos with their differences would be helpful. Otherwise, if we already have reply-filled, cloud-filled, and sidebar-filled icons, why are we switching away?

They're the same icons, they're just being broken apart into separate icon packs.

Minor, but comparing this diff to the current structure of the SWMansion icon pack in the codebase, there are some differences. For example, you wrote CommIcons.ttf whereas currently it's swmansion.ttf (all lowercase). Personally, I think CommIcons.ttf is better, especially looking at the rest of the Info.release.plist file which contains useful capitalization, but maybe the swmansion.ttf can be renamed to SWMansion.ttf or something. Or even include the word icons in it, like with CommIcons.ttf.

I was planning on changing swmansion.ttf to SWMansion.ttf when I got to it.