[web] Update `SWMansionIcon` component with cleaned up icons
ClosedPublic
Actions

Authored by atul on Aug 17 2022, 7:43 PM.

Details

Reviewers

• abosh
varun
tomek
• jacek

Commits

rCOMM5081f25f39a9: [web] Update `SWMansionIcon` component with cleaned up icons

Summary

Same as D4865, but for web instead of native.

Copied from D4865:

It took a while to clean up the source vectors (converting strokes to outlines, joining paths w/ union, subtract, intersect, difference properly, flattening things, getting alignment correct, getting things exported properly, etc).

Once that was all figured out, imported the cleaned up icons into IcoMoon and prepared for export. Retrieved the relevant TTF fonts and JSON config from IcoMoon and replaced the existing fonts/config in the project.

Everything continues to look as expected (or better), but a few icon names have changed. I'll add the icon name changes to this diff to avoid spamming Phabricator with a bunch of rename diffs.

We're using the same swmansion-icon-config.json file exported from IcoMoon on both web and native now which is great because it means icons across platforms are "backed" by the same source vectors and kept in sync.

We're now including the entire SWMansion icon pack (all 275 icons) so we should hopefully never have to look at this again. If we have additional icons we want to include in the future, they'll live in (the much smaller) CommIcons pack.

Test Plan

WARNING: The icons have some visual artifacts (same as encountered in D4864) that are fixed in D4874.

Tested the web app in Safari/Chrome and the icons appeared as expected. There are a handful of icons that don't appear because their names have changed... I went through and replaced the old icon names with the corresponding new ones.

But other than that, it looks like this change went smoothly.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

atul created this revision.Aug 17 2022, 7:43 PM

Herald added a subscriber: ashoat. · View Herald TranscriptAug 17 2022, 7:43 PM

Harbormaster completed remote builds in B11433: Diff 15728.Aug 17 2022, 7:54 PM

atul requested review of this revision.Aug 17 2022, 7:54 PM

atul planned changes to this revision.Aug 17 2022, 10:05 PM

update flow type + instances of old icons

atul edited the summary of this revision. (Show Details)Aug 18 2022, 10:31 AM

atul edited the test plan for this revision. (Show Details)

Harbormaster completed remote builds in B11448: Diff 15745.Aug 18 2022, 10:39 AM

atul added inline comments.Aug 18 2022, 10:41 AM

web/SWMansionIcon.react.js
21–27 ↗	(On Diff #15745)	Generated the `flow` type with the following generate_icon_type.py import os arr = os.listdir('cleaned_vectors') arr.sort() for each in arr: fname = each.split(".svg")[0] print(f"'{fname}' \|")
web/chat/chat-input-bar.react.js
230 ↗	(On Diff #15745)	Same icon, different name
253 ↗	(On Diff #15745)	Same icon, different name
web/chat/chat-tabs.react.js
48 ↗	(On Diff #15745)	Using `home-1` icon to match what we're doing on `native` Here's a before/after
web/chat/chat-thread-list-item.react.js
119 ↗	(On Diff #15745)	Instead of `chevron-right-small` which isn't in the icon pack, just reduced the size and added `span.breadCrumb svg` selector to `web/chat/chat-thread-list.css` to bring back the horizontal padding.
web/chat/thread-menu.react.js
59 ↗	(On Diff #15745)	The web/desktop designs on Figma interchangeably use the `wrench` icon and `settings` (cog) icon. We use `settings` throughout on the `native` app so went with that for consistency. Here's a before/after
110 ↗	(On Diff #15745)	This just looks strictly better and matches the proportion of existing icons in the menu. Here's a before/after
web/modals/threads/members/member.react.js
131 ↗	(On Diff #15745)	Same icon, different name
web/modals/threads/settings/thread-settings-privacy-tab.react.js
131 ↗	(On Diff #15745)	Same icon, different name
web/settings/account-settings.react.js
77 ↗	(On Diff #15745)	Same icon, different name
web/settings/relationship/block-list-row.react.js
16 ↗	(On Diff #15745)	Same icon, different name
web/settings/relationship/friend-list-row.react.js
48 ↗	(On Diff #15745)	Same icon, different name
web/sidebar/app-switcher.react.js
117 ↗	(On Diff #15745)	Same icon, different name

atul attached a referenced file: F143143: Screen Shot 2022-08-18 at 1.40.42 PM.png. (Show Details)Aug 18 2022, 10:59 AM

atul attached a referenced file: F143140: Screen Shot 2022-08-18 at 1.39.03 PM.png. (Show Details)

atul attached a referenced file: F143137: Screen Shot 2022-08-18 at 1.35.58 PM.png. (Show Details)

atul added 2 blocking reviewer(s): • abosh, varun.

atul edited the test plan for this revision. (Show Details)Aug 18 2022, 12:01 PM

atul added a child revision: D4874: [web] Set `stroke: none` for `SWMansionIcon` SVGs.

Thanks for the inline comments, those are really useful. Maybe one of the diffs in an icons stack could be moved to the Diff Hall of Fame 👀

Additionally, clarified IRL with you, but this should wait to be landed until D4874 is landed since we want to update the icons once we've set the stroke: none property for SWMansion SVGs to avoid introducing artifacts/regressions.

web/SWMansionIcon.react.js
21–27 ↗	(On Diff #15745)	Nice! I wonder if something like GitHub Copilot could figure out how to write the type if you wrote the first couple of lines (with the SVGs in the same directory as this file), like `'air' \| 'alarm' \| 'arrow-circle-down'` and let it do the rest.