Integrate support for SVG icons and fix clear icon #15691
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,29 +1,43 @@ | ||
| (ns quo2.components.icon | ||
| (:require [clojure.string :as string] | ||
| [quo2.components.icons.icons :as icons] | ||
| [quo2.components.icons.svg :as icons.svg] | ||
| [quo2.foundations.colors :as colors] | ||
| [react-native.core :as rn])) | ||
|
|
||
| (defn- valid-color? | ||
| [color] | ||
| (or (keyword? color) | ||
| (and (string? color) | ||
| (not (string/blank? color))))) | ||
|
|
||
| (defn memo-icon-fn | ||
| ([icon-name] (memo-icon-fn icon-name nil)) | ||
| ([icon-name | ||
| {:keys [color color-2 no-color | ||
| container-style size accessibility-label] | ||
| :or {accessibility-label :icon}}] | ||
| (let [size (or size 20)] | ||
| ^{:key icon-name} | ||
| (if-let [svg-icon (icons.svg/get-icon icon-name size)] | ||
| [svg-icon | ||
| {:size size | ||
| :color (when (valid-color? color) color) | ||
| :color-2 (when (valid-color? color-2) color-2) | ||
| :accessibility-label accessibility-label | ||
| :style container-style}] | ||
| [rn/image | ||
| {:style | ||
| (merge {:width size | ||
| :height size} | ||
|
|
||
| (when (not no-color) | ||
| {:tint-color (if (and (string? color) (not (string/blank? color))) | ||
| color | ||
| (colors/theme-colors colors/neutral-100 colors/white))}) | ||
|
|
||
| container-style) | ||
| :accessibility-label accessibility-label | ||
| :source (icons/icon-source (str (name icon-name) size))}])))) | ||
|
|
||
| (def icon (memoize memo-icon-fn)) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| (ns quo2.components.icons.svg | ||
| "Declare icons in this namespace when they have two possible colors, because the | ||
| ReactNative `:tint-color` prop affects all non-transparent pixels of PNGs. If | ||
| the icon has only one color, prefer a PNG. | ||
|
|
||
| Keep all SVG components private and expose them by name in the `icons` var." | ||
| (:require [react-native.svg :as svg] | ||
| [quo2.foundations.colors :as colors])) | ||
|
|
||
| (defn- container | ||
| [{:keys [size accessibility-label style] | ||
| :or {size 20}} | ||
| & children] | ||
| (into [svg/svg | ||
| {:accessibility-label accessibility-label | ||
| :style style | ||
| :width size | ||
| :height size | ||
| :view-box (str "0 0 " size " " size) | ||
| :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.
I did use a tool to convert the SVG to hiccup automatically ;) Took me 1min http://html2hiccup.buttercloud.com/
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. |
||
| [{: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 |
||
| (let [color (or color colors/neutral-100) | ||
| color-2 (or color-2 colors/white)] | ||
| [container props | ||
| [svg/path | ||
| {:d | ||
| "M3 10C3 6.13401 6.13401 3 10 3C13.866 3 17 6.13401 17 10C17 13.866 13.866 17 10 17C6.13401 17 3 13.866 3 10Z" | ||
| :fill color}] | ||
| [svg/path | ||
| {:d | ||
| "M9.15142 9.99998L7.07566 12.0757L7.9242 12.9243L9.99994 10.8485L12.0757 12.9242L12.9242 12.0757L10.8485 9.99998L12.9242 7.92421L12.0757 7.07568L9.99994 9.15145L7.92421 7.07572L7.07568 7.92425L9.15142 9.99998Z" | ||
| :fill color-2}]])) | ||
|
|
||
| (def ^:private icons | ||
| {:i/clear-20 clear-20}) | ||
|
|
||
| (defn- append-to-keyword | ||
| [k & xs] | ||
| (keyword (apply str | ||
| (subs (str k) 1) | ||
| xs))) | ||
|
|
||
| (defn get-icon | ||
| [icon-name size] | ||
| (get icons (append-to-keyword icon-name "-" size))) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to use hardcoded size here? Is it better to have a size options like in the
containerabove?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
containercomponent.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.dprop, but I felt this was out of scope for this issue.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.