Page MenuHomePhabricator

[web] Set `stroke: none` for `SWMansionIcon` SVGs
ClosedPublic

Authored by atul on Aug 18 2022, 10:52 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 22, 10:47 AM
Unknown Object (File)
Fri, Nov 22, 10:30 AM
Unknown Object (File)
Fri, Nov 22, 5:35 AM
Unknown Object (File)
Thu, Nov 7, 11:14 PM
Unknown Object (File)
Thu, Nov 7, 10:10 PM
Unknown Object (File)
Thu, Nov 7, 10:04 PM
Unknown Object (File)
Thu, Nov 7, 10:02 PM
Unknown Object (File)
Thu, Nov 7, 8:07 PM
Subscribers

Details

Summary

Pretty much the same as D4864, but for SWMansionIcon instead of CommIcon.

Copied from D4864:

Noticed that the SVG icons we're importing via <IcomoonReact> weren't quite as smooth as they could be, specifically noticed some artifacts on the "corners" of the path of the reply-filled icon.

Screen Shot 2022-08-17 at 2.08.39 PM.png (208×412 px, 34 KB)

I tried various approaches (from StackOverflow, blogs, etc) to try to determine what was causing the artifacts and how to resolve the issue (eg setting shape-rendering explicitly).

In experimenting with the icon using the browser developer tools, I noticed that setting stroke to none caused the artifacts to disappear. This was strange because the source SVG for reply-filled we're passing into the <IcomoonReact> component is a single path with a fill and no stroke. So I tried to figure out where the stroke was coming from.

I clicked into node_modules/react-icomoon/src/index.tsx (https://github.com/aykutkardas/react-icomoon/blob/807a42cf61c8e26c078c5818ba0736afc003032a/src/index.tsx#L60) to see what was going on and found that <IcomoonReact> prepares a container for the SVG to "live inside" and sets various SVG properties. Specifically, they set stroke to currentColor. Even though there's no stroke width or whatever, this leads to some visual weirdness when rendered in Safari/Chrome.

Screen Shot 2022-08-17 at 2.36.30 PM.png (260×612 px, 38 KB)

To verify that IcomoonReact setting stroke was the issue, I went into the library's dist folder and tried commenting the line out. After reloading the web app I confirmed that the artifacts no longer appeared.

Screen Shot 2022-08-17 at 2.33.30 PM.png (260×612 px, 33 KB)

Screen Shot 2022-08-17 at 2.10.22 PM.png (208×412 px, 10 KB)

This diff explicitly sets stroke to none in the CommIcon component which passes the style down to IcomoonReact to override the stroke set in the inner component. This change gets rid of the artifacts and the icons look as expected.


Depends on D4866

Test Plan

Things look as expected on Safari/Chrome/Firefox.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

remove accidental newline

web/chat/chat-input-bar.react.js
255 ↗(On Diff #15747)

Verified that this works just as setting color in the style prop did:

Screen Shot 2022-08-18 at 1.49.43 PM.png (138×974 px, 16 KB)

atul retitled this revision from [web] Set `stroke: none` for `SWMansionIcon` SVGs (AKA make icons crispy) to [web] Set `stroke: none` for `SWMansionIcon` SVGs.Aug 18 2022, 10:58 AM
atul requested review of this revision.Aug 18 2022, 11:03 AM
abosh added inline comments.
web/chat/chat-input-bar.react.js
255 ↗(On Diff #15747)

Nice, looks like this shows the color prop works. I'm not sure why it does now, because in D3916 I ran into this same issue where the color prop wasn't doing anything.

web/chat/chat-input-bar.react.js
255 ↗(On Diff #15747)

It's because all the SVGs were fucked up in unique and novel ways

web/chat/chat-input-bar.react.js
255 ↗(On Diff #15747)

Going forward we won't need to do layout/styling hacks on an icon-by-icon basis...

This revision is now accepted and ready to land.Aug 18 2022, 1:02 PM