Integrate support for SVG icons and fix clear icon #15691
Conversation
ilmotta
requested review from
alwx,
erikseppanen,
cammellos,
rasom,
vkjr,
flexsurfer,
qfrank,
yqrashawn,
Parveshdhull,
briansztamfater,
ajayesivan,
smohamedjavid,
J-Son89,
jo-mut and
OmarBasem
Jenkins BuildsClick to see older builds (8)
|
| children)) | ||
|
|
||
| (defn- clear-20 | ||
| [{:keys [color color-2] :as props}] |
There was a problem hiding this comment.
color 2 is like a background color? 🤔
There was a problem hiding this comment.
I initially tried to name color-2 as background-color, just to realize it was more confusing for some icons, like mutual-contact, positive-state, etc, especially when/if we migrate more icons to SVGs I think color-2 is more future proof. It's really like a secondary color, but I didn't want to name it secondary-color or alternate-color, etc, so I ended up going for this simplistic name.
| :fill :none}] | ||
| children)) | ||
|
|
||
| (defn- clear-20 |
There was a problem hiding this comment.
Why do we need to use hardcoded size here? Is it better to have a size options like in the container above?
There was a problem hiding this comment.
@ajayesivan, the SVG exported by Figma only works correctly with a specific dimension, 20 in this case. The SVG generated by Figma for size 12 is different and I checked other icons and it's the same story. So I chose to be as explicit as possible and name the var with the size. The only thing that was safe to generalize was the SVG viewbox, which I extracted into the container component.
Naming the var with the size was the way I found to tell other devs that this icon var should not be used for other sizes.
It is in fact possible to generalize the icons to different sizes by manipulating the path.d prop, but I felt this was out of scope for this issue.
There was a problem hiding this comment.
It's also good that all of these vars are private, and we can safely improve them in the future without causing breakage.
There was a problem hiding this comment.
Ideally, we should be able to use a single SVG for all size variations, and I think thats one of the major advantages of using SVG over PNG. Maybe we can discuss this with design team and come up with a better solution.
There was a problem hiding this comment.
Agree with @ajayesivan, I think that the point of using vectorized icons is to use one resource for all sizes without loosing quality. Ideally we should export it once.
Also wondering if we add lots of icons how big this file will end up being.
There was a problem hiding this comment.
Totally agree with you guys. If we want to replace PNGs with SVGs in the future, we'll need to coordinate with designers a bit of work, especially to optimize them for reusability for different sizes. The exported SVGs from Figma are fine, but not ideal.
| :fill :none}] | ||
| children)) | ||
|
|
||
| (defn- clear-20 |
There was a problem hiding this comment.
i would suggest to move svg icons to a separate folder as js files with component, because no need in having hiccup , yes they are small, but still why, there are also tools for exporting svg as react components etc, it should make life easier later , on other hand we could have a macro in cljs and load svg file as we did this before, in that case we don't need to do any manuall manipulations with svg, just export same as png
There was a problem hiding this comment.
There was a problem hiding this comment.
Sounds good, but I think this is a bit of a next step @flexsurfer. Here in this issue my intention was not to create something that can evolve to replace all PNGs. This would be much bigger in scope and more risky as we discussed, since SVG icons may not work that well in all devices.
After this PR, my hope is that we'll explore SVG icons even more, and hopefully manipulating via hiccup won't be necessary as you said. Hopefully we'll be able to automate the work, just as we did with PNGs :)
There was a problem hiding this comment.
there are also tools for exporting svg as react components
I did use a tool to convert the SVG to hiccup automatically ;) Took me 1min http://html2hiccup.buttercloud.com/
i would suggest to move svg icons to a separate folder as js files with component, because no need in having hiccup , yes they are small, but still why, there are also tools for exporting svg as react components etc, it should make life easier later
If you don't mind too much, I prefer to keep the code as is, since there's only one icon now. If this grows, then sure, let's do something better and throw away the code I wrote in this PR.
This commit solves the problem described in detail in issue #15606, but in essence, it fixes the clear icon by integrating rudimentary support for SVG icons. Fixes #15606 - Hopefully, if SVG icons prove to be a solid solution, we can easily and progressively migrate PNG icons to SVGs, but for the moment, it was aligned with @flexsurfer #15606 (comment) that we'll only use SVG icons on demand. - Note that it's possible to import SVGs directly via js/require by installing the library react-native-svg-transformer, but this approach is only good when we don't want/need color customization, which is rarely the case with icons where we want to change the foreground and/or background colors. I opted for rendering the SVG icon as hiccup to support color customization. - Since icons are fully memoized, the app's performance is on the same ballpark as PNGs rendered with RN Image. - It's possible to trim down SVGs by using a tool such as https://github.com/svg/svgo, but this is obviously outside the scope of this PR.
Fixes #15606
Summary
This PR solves the problem described in detail in the issue #15606, but in essence, it fixes the clear icon by integrating support for SVG icons.
js/requireby installing the libraryreact-native-svg-transformer, but this approach is only good when we don't want/need color customization, which is rarely the case with icons where we want to change the foreground and/or background colors. I opted for rendering the SVG icon as hiccup to support color customization.Image.1. Fix incorrect clear icon in the input chat image list
Before
After
2. Fix incorrect clear icon in the input component
3. Fix incorrect clear icon in the input search component
4. Fix incorrect clear icon in the URL preview component
Before
After
Platforms
status: ready