Create a separate CommIcon pack that includes custom icons that aren't in the SWMansion icon pack.
Here are the included icons:
Details
Details
Will be tested in subsequent diffs that "consume" the CommIcon component.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| native/components/comm-icon.react.js | ||
|---|---|---|
| 7–11 ↗ | (On Diff #15612) | Basically copy/pasted from native/components/swmansion-icon.react.js |
Comment Actions
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.
Comment Actions
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.
Comment Actions
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
Thanks, this was the context I needed!
I was planning on changing swmansion.ttf to SWMansion.ttf when I got to it.
Just quickly did that: D4838.