From 0bff76e5f141eac40359b3fd95bb08f38cc6cba2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Florian=20Schr=C3=B6dl?= Date: Tue, 22 Apr 2025 09:10:07 +0200 Subject: [PATCH] :sparkles: Don't override user provided color format (#6231) --- frontend/playwright/ui/specs/tokens.spec.js | 57 ++++++++++++++++++- .../controls/input_token_color_bullet.cljs | 3 +- .../app/main/ui/workspace/tokens/form.cljs | 45 +++++++++++---- .../main/ui/workspace/tokens/tinycolor.cljs | 19 +++++-- 4 files changed, 105 insertions(+), 19 deletions(-) diff --git a/frontend/playwright/ui/specs/tokens.spec.js b/frontend/playwright/ui/specs/tokens.spec.js index 73e8488316..e717b77e36 100644 --- a/frontend/playwright/ui/specs/tokens.spec.js +++ b/frontend/playwright/ui/specs/tokens.spec.js @@ -371,6 +371,59 @@ test.describe("Tokens: Tokens Tab", () => { await expect(tokensTabPanel.getByLabel("color.dark.primary")).toBeEnabled(); }); + test("User changes color token color while keeping custom color space", async ({ + page, + }) => { + const { workspacePage, tokensUpdateCreateModal, tokenThemesSetsSidebar } = + await setupEmptyTokensFile(page); + + const tokensTabPanel = page.getByRole("tabpanel", { name: "tokens" }); + await tokensTabPanel.getByTitle("Add token: Color").click(); + + await expect(tokensUpdateCreateModal).toBeVisible(); + const nameField = tokensUpdateCreateModal.getByLabel("Name"); + const valueField = tokensUpdateCreateModal.getByLabel("Value"); + + await valueField.click(); + await valueField.fill("hsv(1,1,1)"); + await expect( + tokensUpdateCreateModal.getByText("Resolved value: #ff0400"), + ).toBeVisible(); + + const colorBullet = tokensUpdateCreateModal.getByTestId( + "token-form-color-bullet", + ); + await colorBullet.click(); + + const valueSaturationSelector = tokensUpdateCreateModal.getByTestId( + "value-saturation-selector", + ); + await expect(valueSaturationSelector).toBeVisible(); + + // Check if color space doesnt get overwritten when changing color via the picker + // Not testing for exact value to avoid flakiness of px click + await valueSaturationSelector.click({ position: { x: 100, y: 100 } }); + await expect(valueField).not.toHaveValue("hsv(1,1,1)"); + await expect(valueField).toHaveValue(/^hsv.*$/); + + // Clearing the input field should pick hex + await valueField.fill(""); + await expect( + tokensUpdateCreateModal.getByText("Resolved value: -"), + ).toBeVisible(); + await valueSaturationSelector.click({ position: { x: 50, y: 50 } }); + await expect(valueField).toHaveValue(/^#[A-Fa-f\d]+$/); + + // Changing opacity for hex values converts to rgba + const sliderOpacity = tokensUpdateCreateModal.getByTestId("slider-opacity"); + await sliderOpacity.click({ position: { x: 50, y: 0 } }); + await expect(valueField).toHaveValue(/^rgba(.*)$/); + + // Changing color now will stay in rgba + await valueSaturationSelector.click({ position: { x: 0, y: 0 } }); + await expect(valueField).toHaveValue(/^rgba(.*)$/); + }); + test("User duplicate color token", async ({ page }) => { const { tokensSidebar, tokenContextMenuForToken } = await setupTokensFile(page); @@ -511,7 +564,9 @@ test.describe("Tokens: Sets Tab", () => { // Creates nesting by renaming set with double click await tokenThemesSetsSidebar .getByRole("button", { name: "light-renamed" }) - .dblclick(); + .click({ button: "right" }); + await expect(tokenContextMenuForSet).toBeVisible(); + await tokenContextMenuForSet.getByText("Rename").click(); await changeSetInput(tokenThemesSetsSidebar, "nested/light"); await assertSetsList(tokenThemesSetsSidebar, [ diff --git a/frontend/src/app/main/ui/workspace/tokens/components/controls/input_token_color_bullet.cljs b/frontend/src/app/main/ui/workspace/tokens/components/controls/input_token_color_bullet.cljs index 4ff70d333b..581c7114f3 100644 --- a/frontend/src/app/main/ui/workspace/tokens/components/controls/input_token_color_bullet.cljs +++ b/frontend/src/app/main/ui/workspace/tokens/components/controls/input_token_color_bullet.cljs @@ -20,7 +20,8 @@ {::mf/props :obj ::mf/schema schema::input-token-color-bullet} [{:keys [color on-click]}] - [:div {:class (stl/css :input-token-color-bullet) + [:div {:data-testid "token-form-color-bullet" + :class (stl/css :input-token-color-bullet) :on-click on-click} (if-let [color' (wtt/color-bullet-color color)] [:> color-bullet {:color color' :mini true}] diff --git a/frontend/src/app/main/ui/workspace/tokens/form.cljs b/frontend/src/app/main/ui/workspace/tokens/form.cljs index 9306904317..bf2e70075d 100644 --- a/frontend/src/app/main/ui/workspace/tokens/form.cljs +++ b/frontend/src/app/main/ui/workspace/tokens/form.cljs @@ -198,6 +198,13 @@ (when-not (and dragging? hex) (reset! internal-color* selector-color) (on-change hex alpha)))))] + (mf/use-effect + (mf/deps color) + (fn [] + ;; Update internal color when user changes input value + (when-let [color (tinycolor/valid-color color)] + (when-not (= (tinycolor/->hex-string color) (:hex internal-color)) + (reset! internal-color* (hex->value color)))))) (colorpicker/use-color-picker-css-variables! wrapper-node-ref internal-color) [:div {:ref wrapper-node-ref} @@ -315,7 +322,8 @@ (valid-name? @token-name-ref)) ;; Value - color (mf/use-state (when color? (:value token))) + color* (mf/use-state (when color? (:value token))) + color (deref color*) color-ramp-open* (mf/use-state false) color-ramp-open? (deref color-ramp-open*) value-input-ref (mf/use-ref nil) @@ -338,7 +346,7 @@ :else (:resolved-value token-or-err))] - (when color? (reset! color (if error? nil v))) + (when color? (reset! color* (if error? nil v))) (reset! token-resolve-result* v)))) on-update-value-debounced (use-debonced-resolve-callback token-name-ref token active-theme-tokens set-resolve-value) @@ -355,13 +363,28 @@ (reset! value-ref value') (on-update-value-debounced value')))) on-update-color (mf/use-fn - (mf/deps on-update-value-debounced) + (mf/deps color on-update-value-debounced) (fn [hex-value alpha] - (let [color-value (if (= 1 alpha) - hex-value - (-> (tinycolor/valid-color hex-value) - (tinycolor/set-alpha alpha) - (tinycolor/->rgba-string)))] + (let [;; StyleDictionary will always convert to hex/rgba, so we take the format from the value input field + prev-input-color (some-> (dom/get-value (mf/ref-val value-input-ref)) + (tinycolor/valid-color)) + ;; If the input is a reference we will take the format from the computed value + prev-computed-color (when-not prev-input-color + (some-> color (tinycolor/valid-color))) + prev-format (some-> (or prev-input-color prev-computed-color) + (tinycolor/color-format)) + to-rgba? (and + (< alpha 1) + (or (= prev-format "hex") (not prev-format))) + to-hex? (and (not prev-format) (= alpha 1)) + format (cond + to-rgba? "rgba" + to-hex? "hex" + prev-format prev-format + :else "hex") + color-value (-> (tinycolor/valid-color hex-value) + (tinycolor/set-alpha (or alpha 1)) + (tinycolor/->string format))] (reset! value-ref color-value) (dom/set-value! (mf/ref-val value-input-ref) color-value) (on-update-value-debounced color-value)))) @@ -538,10 +561,10 @@ :on-blur on-update-value} (when color? [:> input-token-color-bullet* - {:color @color :on-click on-display-colorpicker'}])] + {:color color + :on-click on-display-colorpicker'}])] (when color-ramp-open? - [:> ramp* {:color (some-> (or token-resolve-result (:value token)) - (tinycolor/valid-color)) + [:> ramp* {:color (some-> color (tinycolor/valid-color)) :on-change on-update-color}]) [:& token-value-or-errors {:result-or-errors token-resolve-result}]] diff --git a/frontend/src/app/main/ui/workspace/tokens/tinycolor.cljs b/frontend/src/app/main/ui/workspace/tokens/tinycolor.cljs index 10c809e397..4a8a68bf1f 100644 --- a/frontend/src/app/main/ui/workspace/tokens/tinycolor.cljs +++ b/frontend/src/app/main/ui/workspace/tokens/tinycolor.cljs @@ -32,17 +32,24 @@ (let [tc (tinycolor color-str)] (str/starts-with? (.getFormat tc) "hex")))) -(defn ->string [^js tc] - (.toString tc)) +(defn ->string + "Stringify `tc` to `format`, uses `hex` as per default." + [^js tc format] + (let [format' (case format + ;; Tinycolor `.toString` doesnt support the `a` suffix it gives you via `.getFormat` + "rgba" "rgb" + "hsva" "hsv" + ;; Keep these formats + "rgb" "rgb" + "hsv" "hsv" + ;; Fall back to hex as the default + "hex")] + (.toString tc format'))) (defn ->hex-string [^js tc] (assert (tinycolor? tc)) (.toHexString tc)) -(defn ->rgba-string [^js tc] - (assert (tinycolor? tc)) - (.toRgbString tc)) - (defn color-format [^js tc] (assert (tinycolor? tc)) (.getFormat tc))