From c728a02f304c685190ac94cf7676f3242bd35c81 Mon Sep 17 00:00:00 2001 From: JC Franco Date: Mon, 31 Jul 2023 11:24:45 -0700 Subject: [PATCH 1/4] fix(color-picker): draw slider thumbs within bounds --- .../color-picker/color-picker.e2e.ts | 32 ++++++++++--------- .../components/color-picker/color-picker.tsx | 29 +++++++++++------ .../calcite-components/src/utils/math.spec.ts | 10 +++++- packages/calcite-components/src/utils/math.ts | 4 +++ 4 files changed, 49 insertions(+), 26 deletions(-) diff --git a/packages/calcite-components/src/components/color-picker/color-picker.e2e.ts b/packages/calcite-components/src/components/color-picker/color-picker.e2e.ts index eb5c0df9e07..3f465d18fcb 100644 --- a/packages/calcite-components/src/components/color-picker/color-picker.e2e.ts +++ b/packages/calcite-components/src/components/color-picker/color-picker.e2e.ts @@ -515,12 +515,12 @@ describe("calcite-color-picker", () => { const expectedColorSamples = [ "#ff0000", "#ffd900", - "#48ff00", - "#00ff91", + "#4cff00", + "#00ff8c", "#0095ff", - "#4800ff", - "#ff00dd", - "#ff0004", + "#4400ff", + "#ff00e1", + "#ff0008", ]; for (let i = 0; i < expectedColorSamples.length; i++) { @@ -672,7 +672,7 @@ describe("calcite-color-picker", () => { [hueScopeX, hueScopeY] = await getElementXY(page, "calcite-color-picker", `.${CSS.hueScope}`); - expect(hueScopeX).toBe(hueSliderX); + expect(hueScopeX).toBe(hueSliderX + DIMENSIONS.m.thumb.radius); await page.mouse.move(hueScopeX, hueScopeY); await page.mouse.down(); @@ -682,7 +682,7 @@ describe("calcite-color-picker", () => { [hueScopeX] = await getElementXY(page, "calcite-color-picker", `.${CSS.hueScope}`); - expect(hueScopeX).toBe(hueSliderX + DIMENSIONS.m.slider.width - 1); + expect(hueScopeX).toBe(hueSliderX + DIMENSIONS.m.slider.width - DIMENSIONS.m.thumb.radius); }); describe("unsupported value handling", () => { @@ -2187,25 +2187,27 @@ describe("calcite-color-picker", () => { } }; - const getScopeLeftOffset = async () => parseFloat((await scope.getComputedStyle()).left); + const getScopeLeftOffset = async () => + /* use int to simplify test */ + parseInt((await scope.getComputedStyle()).left); - expect(await getScopeLeftOffset()).toBe(0); + expect(await getScopeLeftOffset()).toBe(0 + DIMENSIONS.m.thumb.radius); await nudgeAQuarterOfSlider(); - expect(await getScopeLeftOffset()).toBe(68); + expect(await getScopeLeftOffset()).toBe(71); await nudgeAQuarterOfSlider(); - expect(await getScopeLeftOffset()).toBe(136); + expect(await getScopeLeftOffset()).toBe(133); await nudgeAQuarterOfSlider(); // hue wraps around, so we nudge it back to assert position at the edge await scope.press("ArrowLeft"); - expect(await getScopeLeftOffset()).toBeLessThanOrEqual(204); - expect(await getScopeLeftOffset()).toBeGreaterThan(203); + expect(await getScopeLeftOffset()).toBeLessThanOrEqual(194); + expect(await getScopeLeftOffset()).toBeGreaterThan(193); // nudge it to wrap around await scope.press("ArrowRight"); - expect(await getScopeLeftOffset()).toBe(0); + expect(await getScopeLeftOffset()).toBe(0 + DIMENSIONS.m.thumb.radius); }); it("allows editing hue slider via keyboard", async () => { @@ -2237,7 +2239,7 @@ describe("calcite-color-picker", () => { expect(await hueSliderScope.getComputedStyle()).toMatchObject({ top: "10px", - left: "0px", + left: `${DIMENSIONS.m.thumb.radius}px`, }); }); }); diff --git a/packages/calcite-components/src/components/color-picker/color-picker.tsx b/packages/calcite-components/src/components/color-picker/color-picker.tsx index 76ab2beba24..3b262bd3be5 100644 --- a/packages/calcite-components/src/components/color-picker/color-picker.tsx +++ b/packages/calcite-components/src/components/color-picker/color-picker.tsx @@ -62,7 +62,7 @@ import { LocalizedComponent, NumberingSystem, } from "../../utils/locale"; -import { clamp } from "../../utils/math"; +import { clamp, remap } from "../../utils/math"; import { connectMessages, disconnectMessages, @@ -618,7 +618,7 @@ export class ColorPicker } else if (clientX < bounds.x) { samplingX = 0; } else { - samplingX = bounds.width - 1; + samplingX = bounds.width; } if (clientY < bounds.y + bounds.height && clientY > bounds.y) { @@ -1074,7 +1074,7 @@ export class ColorPicker slider: { width }, }, } = this; - const hue = (360 / width) * x; + const hue = ((HSV_LIMITS.h - 1) / width) * x; this.internalColorSet(this.baseColorFieldColor.hue(hue), false); } @@ -1364,7 +1364,6 @@ export class ColorPicker const startAngle = 0; const endAngle = 2 * Math.PI; const outlineWidth = 1; - radius = radius - outlineWidth; context.beginPath(); context.arc(x, y, radius, startAngle, endAngle); @@ -1396,14 +1395,15 @@ export class ColorPicker }, } = this; - const x = hsvColor.hue() / (360 / width); + const x = hsvColor.hue() / ((HSV_LIMITS.h - 1) / width); const y = radius - height / 2 + height / 2; + const sliderBoundX = remap(x, 0, width, radius, width - radius); requestAnimationFrame(() => { - this.hueScopeLeft = x; + this.hueScopeLeft = sliderBoundX; }); - this.drawThumb(this.hueSliderRenderingContext, radius, x, y, hsvColor); + this.drawThumb(this.hueSliderRenderingContext, radius, sliderBoundX, y, hsvColor); } private drawHueSlider(): void { @@ -1420,7 +1420,15 @@ export class ColorPicker const gradient = context.createLinearGradient(0, 0, width, 0); - const hueSliderColorStopKeywords = ["red", "yellow", "lime", "cyan", "blue", "magenta", "red"]; + const hueSliderColorStopKeywords = [ + "red", + "yellow", + "lime", + "cyan", + "blue", + "magenta", + "#ff0004" /* 1 unit less than #ff0 to avoid duplicate values within range */, + ]; const offset = 1 / (hueSliderColorStopKeywords.length - 1); let currentOffset = 0; @@ -1544,12 +1552,13 @@ export class ColorPicker const x = alphaToOpacity(hsvColor.alpha()) / (OPACITY_LIMITS.max / width); const y = radius; + const sliderBoundX = remap(x, 0, width, radius, width - radius); requestAnimationFrame(() => { - this.opacityScopeLeft = x; + this.opacityScopeLeft = sliderBoundX; }); - this.drawThumb(this.opacitySliderRenderingContext, radius, x, y, hsvColor); + this.drawThumb(this.opacitySliderRenderingContext, radius, sliderBoundX, y, hsvColor); } private storeOpacityScope = (node: HTMLDivElement): void => { diff --git a/packages/calcite-components/src/utils/math.spec.ts b/packages/calcite-components/src/utils/math.spec.ts index be7eee4b27a..a0c1064e8d8 100644 --- a/packages/calcite-components/src/utils/math.spec.ts +++ b/packages/calcite-components/src/utils/math.spec.ts @@ -1,4 +1,4 @@ -import { clamp, decimalPlaces } from "./math"; +import { clamp, decimalPlaces, remap } from "./math"; describe("clamp", () => { it("clamps numbers within min/max", () => { @@ -14,3 +14,11 @@ describe("decimalPlaces", () => { expect(decimalPlaces(123.123)).toBe(3); }); }); + +describe("remap", () => { + it("remaps numbers", () => { + expect(remap(5, 0, 10, 0, 100)).toBe(50); + expect(remap(0, -100, 100, 0, 100)).toBe(50); + expect(remap(0.5, 0, 1, 0, 100)).toBe(50); + }); +}); diff --git a/packages/calcite-components/src/utils/math.ts b/packages/calcite-components/src/utils/math.ts index 3bc251556bc..5d92e1fe1a9 100644 --- a/packages/calcite-components/src/utils/math.ts +++ b/packages/calcite-components/src/utils/math.ts @@ -15,3 +15,7 @@ export const decimalPlaces = (value: number): number => { (match[2] ? +match[2] : 0) ); }; + +export function remap(value: number, fromMin: number, fromMax: number, toMin: number, toMax: number): number { + return ((value - fromMin) * (toMax - toMin)) / (fromMax - fromMin) + toMin; +} From ef519e0d61f7dfe431384750663390948423a6cc Mon Sep 17 00:00:00 2001 From: JC Franco Date: Wed, 2 Aug 2023 13:30:58 -0700 Subject: [PATCH 2/4] tidy up --- .../src/components/color-picker/color-picker.tsx | 5 +++-- .../src/components/color-picker/resources.ts | 3 +++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/calcite-components/src/components/color-picker/color-picker.tsx b/packages/calcite-components/src/components/color-picker/color-picker.tsx index 66ff63fcb48..fef49f35e94 100644 --- a/packages/calcite-components/src/components/color-picker/color-picker.tsx +++ b/packages/calcite-components/src/components/color-picker/color-picker.tsx @@ -23,6 +23,7 @@ import { DEFAULT_STORAGE_KEY_PREFIX, DIMENSIONS, HSV_LIMITS, + HUE_LIMIT_CONSTRAINED, OPACITY_LIMITS, RGB_LIMITS, SCOPE_SIZE, @@ -1095,7 +1096,7 @@ export class ColorPicker slider: { width }, }, } = this; - const hue = ((HSV_LIMITS.h - 1) / width) * x; + const hue = (HUE_LIMIT_CONSTRAINED / width) * x; this.internalColorSet(this.baseColorFieldColor.hue(hue), false); } @@ -1416,7 +1417,7 @@ export class ColorPicker }, } = this; - const x = hsvColor.hue() / ((HSV_LIMITS.h - 1) / width); + const x = hsvColor.hue() / (HUE_LIMIT_CONSTRAINED / width); const y = radius - height / 2 + height / 2; const sliderBoundX = remap(x, 0, width, radius, width - radius); diff --git a/packages/calcite-components/src/components/color-picker/resources.ts b/packages/calcite-components/src/components/color-picker/resources.ts index 51a4e7357be..7048ecbc433 100644 --- a/packages/calcite-components/src/components/color-picker/resources.ts +++ b/packages/calcite-components/src/components/color-picker/resources.ts @@ -48,6 +48,9 @@ export const HSV_LIMITS = { v: 100, }; +// 0 and 360 represent the same value, so we limit the hue to 359 +export const HUE_LIMIT_CONSTRAINED = HSV_LIMITS.h - 1; + export const OPACITY_LIMITS = { min: 0, max: 100, From a926dc3d6f0e4526f69c3597cc0ead05005e145c Mon Sep 17 00:00:00 2001 From: JC Franco Date: Wed, 2 Aug 2023 14:39:06 -0700 Subject: [PATCH 3/4] map range only if x is within the edge of the slider --- .../components/color-picker/color-picker.tsx | 20 ++++++++++++++----- .../calcite-components/src/utils/math.spec.ts | 18 ++++++++++++++++- packages/calcite-components/src/utils/math.ts | 12 +++++++++++ 3 files changed, 44 insertions(+), 6 deletions(-) diff --git a/packages/calcite-components/src/components/color-picker/color-picker.tsx b/packages/calcite-components/src/components/color-picker/color-picker.tsx index fef49f35e94..f39f75607a9 100644 --- a/packages/calcite-components/src/components/color-picker/color-picker.tsx +++ b/packages/calcite-components/src/components/color-picker/color-picker.tsx @@ -64,7 +64,7 @@ import { LocalizedComponent, NumberingSystem, } from "../../utils/locale"; -import { clamp, remap } from "../../utils/math"; +import { clamp, closeToRangeEdge, remap } from "../../utils/math"; import { connectMessages, disconnectMessages, @@ -1412,14 +1412,14 @@ export class ColorPicker const { dimensions: { - slider: { height, width }, + slider: { width }, thumb: { radius }, }, } = this; const x = hsvColor.hue() / (HUE_LIMIT_CONSTRAINED / width); - const y = radius - height / 2 + height / 2; - const sliderBoundX = remap(x, 0, width, radius, width - radius); + const y = radius; + const sliderBoundX = this.getSliderBoundX(x, width, radius); requestAnimationFrame(() => { this.hueScopeLeft = sliderBoundX; @@ -1574,7 +1574,7 @@ export class ColorPicker const x = alphaToOpacity(hsvColor.alpha()) / (OPACITY_LIMITS.max / width); const y = radius; - const sliderBoundX = remap(x, 0, width, radius, width - radius); + const sliderBoundX = this.getSliderBoundX(x, width, radius); requestAnimationFrame(() => { this.opacityScopeLeft = sliderBoundX; @@ -1583,6 +1583,16 @@ export class ColorPicker this.drawThumb(this.opacitySliderRenderingContext, radius, sliderBoundX, y, hsvColor); } + private getSliderBoundX(x: number, width: number, radius: number): number { + const closeToEdge = closeToRangeEdge(x, width, radius); + + return closeToEdge === 0 + ? x + : closeToEdge === -1 + ? remap(x, 0, width, radius, radius * 2) + : remap(x, 0, width, width - radius * 2, width - radius); + } + private storeOpacityScope = (node: HTMLDivElement): void => { this.opacityScopeNode = node; }; diff --git a/packages/calcite-components/src/utils/math.spec.ts b/packages/calcite-components/src/utils/math.spec.ts index a0c1064e8d8..2bbdd9feb28 100644 --- a/packages/calcite-components/src/utils/math.spec.ts +++ b/packages/calcite-components/src/utils/math.spec.ts @@ -1,4 +1,4 @@ -import { clamp, decimalPlaces, remap } from "./math"; +import { clamp, closeToRangeEdge, decimalPlaces, remap } from "./math"; describe("clamp", () => { it("clamps numbers within min/max", () => { @@ -22,3 +22,19 @@ describe("remap", () => { expect(remap(0.5, 0, 1, 0, 100)).toBe(50); }); }); + +describe("closeToRangeEdge", () => { + it("returns -1 if close to lower edge", () => { + expect(closeToRangeEdge(0, 100, 10)).toBe(-1); + expect(closeToRangeEdge(9, 100, 10)).toBe(-1); + }); + + it("returns 1 if close to upper edge", () => { + expect(closeToRangeEdge(100, 100, 10)).toBe(1); + expect(closeToRangeEdge(91, 100, 10)).toBe(1); + }); + + it("returns 0 if not close to edge", () => { + expect(closeToRangeEdge(50, 100, 10)).toBe(0); + }); +}); diff --git a/packages/calcite-components/src/utils/math.ts b/packages/calcite-components/src/utils/math.ts index 5d92e1fe1a9..8698bb5da96 100644 --- a/packages/calcite-components/src/utils/math.ts +++ b/packages/calcite-components/src/utils/math.ts @@ -19,3 +19,15 @@ export const decimalPlaces = (value: number): number => { export function remap(value: number, fromMin: number, fromMax: number, toMin: number, toMax: number): number { return ((value - fromMin) * (toMax - toMin)) / (fromMax - fromMin) + toMin; } + +/** + * Helper to determine if a value is close to the edge of a range within a threshold. + * + * @param value + * @param range + * @param threshold + * @returns -1 if close to lower edge, 1 if close to upper edge, 0 otherwise. + */ +export function closeToRangeEdge(value: number, range: number, threshold: number): number { + return value < threshold ? -1 : value > range - threshold ? 1 : 0; +} From 8fd0c57b2288df8bb7e19e66f566fdf3d7d1ed87 Mon Sep 17 00:00:00 2001 From: JC Franco Date: Wed, 2 Aug 2023 19:09:43 -0700 Subject: [PATCH 4/4] fix test --- .../src/components/color-picker/color-picker.e2e.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/calcite-components/src/components/color-picker/color-picker.e2e.ts b/packages/calcite-components/src/components/color-picker/color-picker.e2e.ts index 3b4d35eaeb9..9bf305b8003 100644 --- a/packages/calcite-components/src/components/color-picker/color-picker.e2e.ts +++ b/packages/calcite-components/src/components/color-picker/color-picker.e2e.ts @@ -2200,16 +2200,16 @@ describe("calcite-color-picker", () => { expect(await getScopeLeftOffset()).toBeCloseTo(DIMENSIONS.m.thumb.radius - 0.5, 0); await nudgeAQuarterOfSlider(); - expect(await getScopeLeftOffset()).toBeCloseTo(71, 0); + expect(await getScopeLeftOffset()).toBeCloseTo(67.5, 0); await nudgeAQuarterOfSlider(); - expect(await getScopeLeftOffset()).toBeCloseTo(133, 0); + expect(await getScopeLeftOffset()).toBeCloseTo(135.5, 0); await nudgeAQuarterOfSlider(); // hue wraps around, so we nudge it back to assert position at the edge await scope.press("ArrowLeft"); - expect(await getScopeLeftOffset()).toBeLessThanOrEqual(194); - expect(await getScopeLeftOffset()).toBeGreaterThan(193); + expect(await getScopeLeftOffset()).toBeLessThanOrEqual(193.5); + expect(await getScopeLeftOffset()).toBeGreaterThan(189.5); // nudge it to wrap around await scope.press("ArrowRight");