Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 Fix color wrapper choppiness #5594

Conversation

not-in-stock
Copy link
Contributor

@not-in-stock not-in-stock commented Jan 15, 2025

Fix color picker choppiness

Hue slider doesn't move or moves with unwanted snaps

Steps to reproduce:

  1. Open the color token creation form.
  2. Click on a color to expand the color ramp.
  3. Place the picker circle in the bottom-left corner of the ramp.
  4. Attempt to move the hue slider.

Expected result:

  • The hue slider can be moved.
  • The picker circle moves smoothly across the color ramp.

Actual result:

  • The hue slider cannot be moved.
  • The picker circle exhibits choppy movement when dragged across the color ramp.

Before

Screen.Recording.2025-01-20.at.11.57.28.mov

After

Screen.Recording.2025-01-07.at.10.38.15.mov

@not-in-stock not-in-stock force-pushed the andrei/22-fix-colorpicker-choppiness branch from 106935d to 9f50979 Compare January 16, 2025 10:25
@not-in-stock not-in-stock marked this pull request as ready for review January 16, 2025 16:11
@niwinz
Copy link
Contributor

niwinz commented Jan 23, 2025

I think this is still does not solves the issue:
Screencast from 2025-01-23 09-20-18.webm

I think this issue is there because of the differences between penpot colorpicker and tokens colors management, that uses tinycolor (maybe?). I'm still researching on it.

@niwinz
Copy link
Contributor

niwinz commented Jan 23, 2025

With this changes I have it working correctly:

diff --git a/common/src/app/common/colors.cljc b/common/src/app/common/colors.cljc
index 932d79d63..64a940ad9 100644
--- a/common/src/app/common/colors.cljc
+++ b/common/src/app/common/colors.cljc
@@ -248,11 +248,12 @@
   [[h s brightness]]
   (if (= s 0)
     [brightness brightness brightness]
-    (let [sextant   (int (mth/floor (/ h 60)))
-          remainder (- (/ h 60) sextant)
-          val1      (int (* brightness (- 1 s)))
-          val2      (int (* brightness (- 1 (* s remainder))))
-          val3      (int (* brightness (- 1 (* s (- 1 remainder)))))]
+    (let [sextant    (int (mth/floor (/ h 60)))
+          remainder  (- (/ h 60) sextant)
+          brightness (d/nilv brightness 0)
+          val1       (int (* brightness (- 1 s)))
+          val2       (int (* brightness (- 1 (* s remainder))))
+          val3       (int (* brightness (- 1 (* s (- 1 remainder)))))]
       (case sextant
         1 [val2 brightness val1]
         2 [val1 brightness val3]
diff --git a/frontend/src/app/main/ui/workspace/colorpicker/ramp.cljs b/frontend/src/app/main/ui/workspace/colorpicker/ramp.cljs
index 43565b10d..28d99040a 100644
--- a/frontend/src/app/main/ui/workspace/colorpicker/ramp.cljs
+++ b/frontend/src/app/main/ui/workspace/colorpicker/ramp.cljs
@@ -7,6 +7,7 @@
 (ns app.main.ui.workspace.colorpicker.ramp
   (:require-macros [app.main.style :as stl])
   (:require
+   [app.common.data :as d]
    [app.common.colors :as cc]
    [app.common.math :as mth]
    [app.main.ui.components.color-bullet :as cb]
@@ -52,43 +53,49 @@
 
 
 (defn enrich-color-map [{:keys [h s v] :as color}]
-  (let [[r g b] (cc/hsv->rgb [h s v])]
+  (let [h (d/nilv h 0)
+        s (d/nilv s 0)
+        v (d/nilv v 0)
+        hsv [h s v]
+        [r g b] (cc/hsv->rgb hsv)]
     (assoc color
-           :hex (cc/hsv->hex [h s v])
+           :hex (cc/hsv->hex hsv)
+           :h h :s s :v v
            :r r :g g :b b)))
 
-(mf/defc ramp-selector [{:keys [color disable-opacity on-change on-start-drag on-finish-drag]}]
-  (let [internal-color (mf/use-state)
+(mf/defc ramp-selector
+  [{:keys [color disable-opacity on-change on-start-drag on-finish-drag]}]
+  (let [internal-color
+        (mf/use-state #(enrich-color-map color))
+
         {:keys [h s v hex alpha]} @internal-color
         on-change-value-saturation
         (fn [new-saturation new-value]
           (let [new-color (-> @internal-color
-                              enrich-color-map
-                              (assoc :s new-saturation
-                                     :v new-value))]
+                              (assoc :s new-saturation)
+                              (assoc :v new-value)
+                              (enrich-color-map))]
             (reset! internal-color new-color)
             (on-change new-color)))
 
         on-change-hue
         (fn [new-hue]
           (let [new-color (-> @internal-color
-                              enrich-color-map
-                              (assoc :h new-hue))]
+                              (assoc :h new-hue)
+                              enrich-color-map)]
             (reset! internal-color new-color)
             (on-change new-color)))
 
         on-change-opacity
         (fn [new-opacity]
           (let [new-color (-> @internal-color
-                              enrich-color-map
                               (assoc :alpha new-opacity))]
             (reset! internal-color new-color)
             (on-change new-color)))]
 
-    (mf/use-effect
-     (mf/deps (:hex (enrich-color-map color)))
-     (fn []
-       (reset! internal-color (enrich-color-map color))))
+    (mf/with-effect [color]
+      (reset! internal-color (enrich-color-map color)))
+
     [:*
      [:& value-saturation-selector
       {:hue h

@niwinz
Copy link
Contributor

niwinz commented Jan 23, 2025

If it is ok for you, I can merge all this together

@niwinz
Copy link
Contributor

niwinz commented Jan 23, 2025

I have created a separated PR, with my changes and other related changes, feel free to close this PR if you are ok with changes i have made

@not-in-stock
Copy link
Contributor Author

If it is ok for you, I can merge all this together
@niwinz Great! Let's merge it then. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants