remove custom-color-by-theme method, add resolve-color method #17567
Conversation
J-Son89
requested review from
Parveshdhull,
OmarBasem,
briansztamfater,
ajayesivan,
smohamedjavid and
ulisesmac
| @@ -94,17 +93,15 @@ | |||
| :active-count 112100 | |||
| :unread-count 5})] | |||
| (fn [] | |||
| (let [customization-color (colors/custom-color-by-theme (:customization-color @state) 50 60)] | |||
There was a problem hiding this comment.
the resolving of custom colors should be done internally in the component
| @@ -40,8 +39,7 @@ | |||
| :customization-color :blue | |||
| :locked? nil})] | |||
| (fn [] | |||
| (let [customization-color (colors/custom-color-by-theme (:customization-color @state) 50 60)] | |||
There was a problem hiding this comment.
the resolving of custom colors should be done internally in the component
| @@ -223,8 +223,7 @@ | |||
| 60 "#BD1E56"}}) | |||
|
|
|||
| ;;;; Networks | |||
|
|
|||
| (def networks | |||
| (def ^:private networks | |||
There was a problem hiding this comment.
we should have the maps private so the mechanism is the way to interact with colors 👍
Jenkins BuildsClick to see older builds (28)
|
J-Son89
changed the title
There was a problem hiding this comment.
Great to see we're touching the colors API.
If there is opacity set then we only use the 50 suffix regardless of theme.
I think we just need to be careful because if this trick based on the opacity stops being correct, we'll need to change a lot of code to pass a theme instead of nil. Because there's nothing stopping designers to make themeable colors with opacity in the future.
Another way to look at the resolve-color function is that the consumer should not know if the theme is needed or not internally, so the caller always pass the theme. This is my preferred choice because it's more future proof, we would change the function to be:
(defn resolve-color
([color theme]
(do-resolve-color color theme nil))
([color theme opacity]
...))
src/quo2/foundations/colors.cljs
Outdated
| (alpha resolved-color (/ opacity 100)) | ||
| resolved-color)))))) | ||
|
|
||
| (def ^{:deprecated true :superseded-by "resolve-colors"} |
There was a problem hiding this comment.
Typo: should be resolve-color
src/quo2/foundations/colors.cljs
Outdated
| color hex string or keyword (resolves from custom, network and semantic colors) | ||
| theme :light/:dark | ||
| opacity 0-100 (optional) - if set theme is ignored and goes to 50 suffix internally" | ||
| (memoize |
There was a problem hiding this comment.
It's better to define two functions, one memoized, the other normal/non-memoized, similar to quo2.components.icon. In many occasions (in status-mobile repo or not) I had to do this work manually while in the REPL because I didn't want the function to return the same memoized value.
For instance, for this particular color namespace, if I'm changing any of the vars used by resolve-color like colors-map or customization, the memoized function won't see the new values, which either forces the developer to delete the memoized call during development or extract to a separate var.
There was a problem hiding this comment.
sure I can add this. it should be like this?
(defn resolve-color
([color theme]
(resolve-color color theme nil))
([color theme opacity]
(let [suffix (cond opacity 50
(= :dark theme) 60
:else 50)
resolve-keyword? (keyword? color)
resolved-color (if resolve-keyword?
(get-from-colors-map color suffix)
color)]
(if opacity
(alpha resolved-color (/ opacity 100))
resolved-color))))
(def memo-resolve-color (memoize resolve-color))
There was a problem hiding this comment.
btw, we should have some guidance of when to use each. or the non memo is just for testing?
There was a problem hiding this comment.
@J-Son89, similar to what we do with the icon component, or the i18n/label function, it's the opposite of the code you shared previously.
The final implementation would be:
- public var
resolve-color - private var
do-resolve-color, ornon-memo-resolve-color, orresolve-color-1(quite common in the lisp tradition), etc.
Having the non-memoized function defined separately from the public & memoized one gives the developer the chance to change implementations in the REPL more conveniently. It's not a big deal, but I think it's a sensible thing to do, a good practice.
For instance:
resolve-colordepends oncolors-map, andcolors-mapdepends oncustomizationandnetworks.- If in a REPL session I call
(resolve-color :blue :light), the value will be cached in memory, and if I call with the same args again, the same value will be returned. - If I go ahead and change what
:bluemeans in thecolors-map, evaluate in the REPL the newcolors-mapvar, the new blue value won't be used becauseresolve-colorwill return the cached value. For the new value to be returned, I need now to also re-evaluate the varresolve-colorto reset the memoization cache. This makes iterating in the implementation less convenient, and sometimes frustrating because the dev thinks the implementation has a bug, when in fact it was just using cached results. This flow I'm sharing is happening outside the write file + rerun test, or the hot-reload, here I'm just using the REPL without even saving the file.
I hope what I said makes sense :)
There was a problem hiding this comment.
The other reason could be for testing as you said, although I think it's fine for resolve-color. One could argue that it's better to test the pure function only, and ignore the stateful/memoized one, since testing with memoization is just re-testing the core Clojure function behavior, which could be seen as pointless.
There was a problem hiding this comment.
@J-Son89 I agree about add a separate def for the memoized version. In production code we will always use the memoized one and just for testing the one that is not memoized.
I'd like to suggest a refactoring:
(defn- resolve-color*
([color theme]
(resolve-color* color theme nil))
([color theme opacity]
(let [suffix (if (or opacity (= theme :light))
50
60)]
(cond-> color
(keyword? color) (get-from-colors-map suffix)
opacity (alpha (/ opacity 100))))))
(defn resolve-color (memoize resolve-color*))The problem is I feel a little uncomfortable with the suffix var since we are calculating it even if we might not use it (it'll be unused if (keyword? color) is false 🤔). Instead, if we depend on the value of suffix:
(defn- resolve-color*
([color theme]
(resolve-color* color theme nil))
([color theme opacity]
(let [suffix (cond
(not (keyword? color)) nil
(or opacity (= theme :light)) 50
:else 60)]
(cond-> color
suffix (get-from-colors-map suffix)
opacity (alpha (/ opacity 100))))))I think this one is not as clear as the previous one, as always, there are trade-offs.
IDK, just a suggestion, you could pick whatever you like more or just keep the current implementation.
There was a problem hiding this comment.
ok I like this idea, neat! 🧠
src/quo2/foundations/colors.cljs
Outdated
| (let [suffix (cond opacity 50 | ||
| (= :dark theme) 60 | ||
| :else 50) | ||
| hex? (not (keyword? color)) |
There was a problem hiding this comment.
This check is confusing. If I call (resolve-color "blue"), the hex? binding will be true, but it's not a hex color. Suggestion: rename from hex? to unknown-color?.
There was a problem hiding this comment.
I don't know about that, we should not be passing a string unless it has # at the start, perhaps we can add some validation on it :)
There was a problem hiding this comment.
I agree we shouldn't be passing a valid color, but that's what the code reads in the PR. It says hex?, but the check is only verifying it's not a keyword. In case we want to be a bit more precise in the validation, we have the function hex-string?. I guess we could use it, right?
There was a problem hiding this comment.
Yeah definitely, I will add that!
There was a problem hiding this comment.
hmm, I need to double check about adding this. I will say a function like this will benefit a lot from Mali 👍
src/quo2/foundations/colors.cljs
Outdated
| ([color] | ||
| (resolve-color color nil nil)) | ||
| ([color theme-or-opacity] | ||
| (if (keyword? theme-or-opacity) |
There was a problem hiding this comment.
I believe this function is more complicated than it needs to be. The following diff shows a simplified version that passes the unit tests.
I did 3 things:
- Normalize the
colors-mapvar where all colors are maps that can vary based on the 50/60 opacity. Once normalized, we don't need special conditions to check if the color is a string hex or not. There's no perf hit here, because the function to resolve the color is memoized anyway. - Made the 2-arity version simpler by not checking if the second arg is a keyword or not. I think it's just fine to pass nil as the second argument when calling
(resolve-color some-color nil 50). I prefer this way because there's no surprise. The dev doesn't need to remember that the second arg may be an opacity or theme. Mixing types can be confusing. I don't feel strong about this point, but I tend to prefer explicitness. - Extracted the non-memoized function. It's common to prefix with
do-, but others might prefer other names.
@@ -224,14 +224,14 @@
;;;; Networks
(def ^:private networks
- {:ethereum "#758EEB"
- :optimism "#E76E6E"
- :arbitrum "#6BD5F0"
- :zkSync "#9FA0FE"
- :hermez "#EB8462"
- :xDai "#3FC0BD"
- :polygon "#AD71F3"
- :unknown "#EEF2F5"})
+ {:ethereum {50 "#758EEB" 60 "#758EEB"}
+ :optimism {50 "#E76E6E" 60 "#E76E6E"}
+ :arbitrum {50 "#6BD5F0" 60 "#6BD5F0"}
+ :zkSync {50 "#9FA0FE" 60 "#9FA0FE"}
+ :hermez {50 "#EB8462" 60 "#EB8462"}
+ :xDai {50 "#3FC0BD" 60 "#3FC0BD"}
+ :polygon {50 "#AD71F3" 60 "#AD71F3"}
+ :unknown {50 "#EEF2F5" 60 "#EEF2F5"}})
(def ^:private colors-map
(merge {:primary {50 primary-50
@@ -257,31 +257,26 @@
[s]
(and (string? s) (string/starts-with? s "#")))
+(defn- do-resolve-color
+ ([color]
+ (do-resolve-color color nil nil))
+ ([color theme]
+ (do-resolve-color color theme nil))
+ ([color theme opacity]
+ (let [suffix (cond opacity 50
+ (= :dark theme) 60
+ :else 50)
+ resolved-color (get-in colors-map [color suffix] color)]
+ (if opacity
+ (alpha resolved-color (/ opacity 100))
+ resolved-color))))
+
(def resolve-color
"(resolve-color color theme opacity)
color hex string or keyword (resolves from custom, network and semantic colors)
theme :light/:dark
opacity 0-100 (optional) - if set theme is ignored and goes to 50 suffix internally"
- (memoize
- (fn
- ([color]
- (resolve-color color nil nil))
- ([color theme-or-opacity]
- (if (keyword? theme-or-opacity)
- (resolve-color color theme-or-opacity nil)
- (resolve-color color nil theme-or-opacity)))
- ([color theme opacity]
- (let [suffix (cond opacity 50
- (= :dark theme) 60
- :else 50)
- hex? (not (keyword? color))
- resolved-color (cond hex? color
- (hex-string? (get colors-map color)) (get colors-map color)
- :else (get-in colors-map
- [color suffix]))]
- (if opacity
- (alpha resolved-color (/ opacity 100))
- resolved-color))))))
+ (memoize do-resolve-color))
(def ^{:deprecated true :superseded-by "resolve-colors"}
custom-colorThere was a problem hiding this comment.
Yeah we can normalise the network colors, if kind of breaks the mould a bit however as there is no -60 network colors in the foundations file.
Another thing is we need the hex string for community colors as that's how they come from the backend
There was a problem hiding this comment.
Yeah we can normalise the network colors, if kind of breaks the mould a bit however as there is no -60 network colors in the foundations file.
Exactly, tradeoffs.
Another thing is we need the hex string for community colors as that's how they come from the backend
Could you explain this part @J-Son89? I kind of understand.
About the implementation I suggested, another improvement is to avoid calling twice (get colors-map color). This one also passes the unit tests.
(defn- do-resolve-color
([color theme]
(do-resolve-color color theme nil))
([color theme opacity]
(let [suffix (cond opacity 50
(= :dark theme) 60
:else 50)
resolved (if (hex-string? color)
color
(let [maybe-resolved (get colors-map color)]
(if (hex-string? maybe-resolved)
maybe-resolved
(get-in colors-map [color suffix]))))]
(if opacity
(alpha resolved (/ opacity 100))
resolved))))
(def resolve-color (memoize do-resolve-color))There was a problem hiding this comment.
Oh, apologies I also meant to revert this -
'''
(if (keyword? theme-or-opacity)
-
(resolve-color color theme-or-opacity nil) -
(resolve-color color nil theme-
'''
It was just an idea I was experimenting with before discussing with you. D'oh! 🤦♂️
There was a problem hiding this comment.
With community colors, the creator of a community can make that any hex string they like. However with user colors, network colors and chat colors these are all from a predefined palette decides by the design team. Hopefully that's a bit clearer 😅
Sure I'm happy to have this future proofed 👌 |
|
[Not within the scope of your PR to fix] @J-Son89, since we're reviewing some color stuff, and maybe this is a question others also have. Do you know why certain color vars are defined in I didn't review everything in the namespace, but found these colors for example:
I believe these colors should live outside the |
sure, I will get this addressed. I have a lot of work to continue to do here to organise this colors better so this is good to include in that set of work. |
| @@ -83,7 +83,7 @@ | |||
| :width (:size opts) | |||
| :borderRadius (style/get-border-radius (:size opts)) | |||
| :borderWidth 0.8 | |||
| :backgroundColor (colors/custom-color-by-theme (:customization-color opts) 50 50 10 10)}) | |||
| :backgroundColor (colors/resolve-color (:customization-color opts) :light 10)}) | |||
There was a problem hiding this comment.
I like the word resolve you chose, and I think the code would be just as clear with the more succinct (colors/resolve ...). If you do this, you'll need to use (:refer-clojure :exclude [resolve]).
There was a problem hiding this comment.
Nice work! @J-Son89 🚀🚀🚀
It's good to see the notion doc is getting into action. 😄 🎉
src/quo2/foundations/colors.cljs
Outdated
| (defn- get-from-colors-map | ||
| [color suffix] | ||
| (if (hex-string? (get colors-map color)) | ||
| (get colors-map color) | ||
| (get-in colors-map [color suffix]))) |
There was a problem hiding this comment.
I think it's not a good idea to take a variable number of args inside the function if a condition becomes true, i.e. we only use suffix if the result of (hex-string? ...) is true.
So I'd suggest splitting into to functions:
(defn- get-from-colors-map
([color]
(get colors-map color))
([color suffix]
(get-in colors-map [color suffix])))We are using both params when provided, it doesn't matter if it's not a hex string, since:
(get "#a hex string" 60)
;; => nilso we are safe.
And the previous suggestion leads me to think in a more simple function to extract the values:
(defn- get-from-colors-map [& path]
(get-in colors-map path))@J-Son89 WDYT?
There was a problem hiding this comment.
this is kind of needed for structures like network colors map to work ->
(def ^:private networks
{:ethereum "#758EEB"
:optimism "#E76E6E"
:arbitrum "#6BD5F0"
:zkSync "#9FA0FE"
:hermez "#EB8462"
:xDai "#3FC0BD"
:polygon "#AD71F3"
:unknown "#EEF2F5"})
the alternative is to normalise the data such as @ilmotta suggested but tbh I don't like this as it breaks what is in the foundations file and I prefer to match the mechanism the designers have provided so this system is in some sense self documenting then.
see https://www.figma.com/file/v98g9ZiaSHYUdKWrbFg9eM/Foundations?type=design&node-id=619-10144&mode=dev
There was a problem hiding this comment.
I agree with that, I prefer to not having the data normalized.
And, I was aware of our data structure, the last code I suggested (the get-in version) works for both maps (i.e. the ones with 50/60 and the ones with a string).
src/quo2/foundations/colors.cljs
Outdated
| color hex string or keyword (resolves from custom, network and semantic colors) | ||
| theme :light/:dark | ||
| opacity 0-100 (optional) - if set theme is ignored and goes to 50 suffix internally" | ||
| (memoize |
There was a problem hiding this comment.
@J-Son89 I agree about add a separate def for the memoized version. In production code we will always use the memoized one and just for testing the one that is not memoized.
I'd like to suggest a refactoring:
(defn- resolve-color*
([color theme]
(resolve-color* color theme nil))
([color theme opacity]
(let [suffix (if (or opacity (= theme :light))
50
60)]
(cond-> color
(keyword? color) (get-from-colors-map suffix)
opacity (alpha (/ opacity 100))))))
(defn resolve-color (memoize resolve-color*))The problem is I feel a little uncomfortable with the suffix var since we are calculating it even if we might not use it (it'll be unused if (keyword? color) is false 🤔). Instead, if we depend on the value of suffix:
(defn- resolve-color*
([color theme]
(resolve-color* color theme nil))
([color theme opacity]
(let [suffix (cond
(not (keyword? color)) nil
(or opacity (= theme :light)) 50
:else 60)]
(cond-> color
suffix (get-from-colors-map suffix)
opacity (alpha (/ opacity 100))))))I think this one is not as clear as the previous one, as always, there are trade-offs.
IDK, just a suggestion, you could pick whatever you like more or just keep the current implementation.
| (defn- view-internal | ||
| [{:keys [theme]}] | ||
| (let [logged-in? (rf/sub [:multiaccount/logged-in?]) | ||
| has-profiles? (boolean (rf/sub [:profile/profiles-overview])) | ||
| root (if has-profiles? :profiles :intro) | ||
| light? (= (theme/get-theme) :light)] | ||
| light? (= theme :light)] | ||
| [quo/page-nav | ||
| {:type :title | ||
| :title "Quo2 components preview" | ||
| :text-align :left | ||
| :icon-name :i/close | ||
| :right-side [{:icon-name (if light? :i/dark :i/light) | ||
| :on-press #(if light? (theme/set-theme :dark) (theme/set-theme :light))}] | ||
| :on-press #(if light? (quo.theme/set-theme :dark) (quo.theme/set-theme :light))}] | ||
| :on-press #(if (or logged-in? (not= (rf/sub [:view-id]) :quo2-preview)) | ||
| (rf/dispatch [:navigate-back]) | ||
| (do | ||
| (theme/set-theme :dark) | ||
| (quo.theme/set-theme :dark) |
J-Son89
force-pushed
the
jc/rename-color-system
branch
2 times, most recently
from
24b68e7 to
b47d2c8
Compare
|
@ulisesmac I will move this pr forward to QA as the discussions are only about the internals of the resolve-color mechanism. Which I am happy to adjust ofc! |
84% of end-end tests have passedFailed tests (7)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (36)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
J-Son89
force-pushed
the
jc/rename-color-system
branch
from
b47d2c8 to
fc4e184
Compare
J-Son89
force-pushed
the
jc/rename-color-system
branch
from
fc4e184 to
2c1c258
Compare
|
@J-Son89 thank you! no issues from my side. Ready for design review. cc @Francesca-G |
J-Son89
force-pushed
the
jc/rename-color-system
branch
from
2c1c258 to
43a9fb1
Compare
J-Son89
force-pushed
the
jc/rename-color-system
branch
from
43a9fb1 to
2583143
Compare
J-Son89
force-pushed
the
jc/rename-color-system
branch
from
2583143 to
9e865e0
Compare
This pr continues: #16400
In this pr: I remove the use of custom-color-by-theme method as from discussions with the design team I have realised there is a better approach.
Additionally I have created a method
resolve-colorto replacecustom-colorfunction because we are using this method beyond "custom" colors, it also has uses for community colors, network colors, semantic colors.this method removes the use of internal logic (suffix) and instead moves this to be dependent on theme.
This is possible because we only use the 50 suffix for light and 60 suffix for dark theme
For on-press we use a shadow overlay affect to darken/lighten the color - this is a more generic solution as it works with community colors (hex string).
If there is opacity set then we only use the 50 suffix regardless of theme.
To Test:
I have updated the internal logic to resolve colors of the following components:
avatars/account-avatar
avatars/channel-avatar
avatars/user-avatar
calendar/calendar-day
code/code-snippet
dividers/new-messages
list-items/account-list-card
list-items/channel
list-items/community
list-items/token-value
record-audio/record-audio
selectors/selectors
settings/accounts
wallet/keypair
wallet/network-bridge
wallet/network-link
This was all contained in the quo2 components, probably it will help to have a design review to check the colors too - no component structures etc where changed 👍