HomePhabricator
Diffusion Comm c6c9dcc2d287

[web] Set `stroke: none` for `CommIcon` SVGs

Description

[web] Set stroke: none for CommIcon SVGs

Summary:
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.

Test Plan: Things look as expected on Safari/Chrome/Safari

Reviewers: abosh, varun, tomek, jacek

Reviewed By: varun

Subscribers: ashoat

Differential Revision: https://phab.comm.dev/D4864

Details

Provenance
atulAuthored on Aug 18 2022, 8:22 AM
Reviewer
varun
Differential Revision
D4864: [web] Set `stroke: none` for `CommIcon` SVGs
Parents
rCOMM2819731c742c: [web] Replace `sidebar` and `reply` icon `MessageTooltip` with `CommIcon`
Branches
Unknown
Tags
Unknown