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.

Sign up for GitHub

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-picker): draw slider thumbs within bounds #7398

Merged
merged 5 commits into from
Aug 3, 2023
Merged

fix(color-picker): draw slider thumbs within bounds #7398

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c728a02
fix(color-picker): draw slider thumbs within bounds
jcfranco Jul 31, 2023
913adea
merge main
jcfranco Aug 1, 2023
ef519e0
tidy up
jcfranco Aug 2, 2023
a926dc3
map range only if x is within the edge of the slider
jcfranco Aug 2, 2023
8fd0c57
fix test
jcfranco Aug 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
157 changes: 81 additions & 76 deletions packages/calcite-components/src/components/color-picker/color-picker.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -519,12 +519,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++) {
Expand Down Expand Up @@ -678,7 +678,7 @@ describe("calcite-color-picker", () => {
[hueScopeX, hueScopeY] = await getElementXY(page, "calcite-color-picker", `.${CSS.hueScope}`);
[hueScopeCenterX, hueScopeCenterY] = getScopeCenter(hueScopeX, hueScopeY);

expect(hueScopeCenterX).toBe(hueSliderX);
expect(hueScopeCenterX).toBe(hueSliderX + DIMENSIONS.m.thumb.radius);

await page.mouse.move(hueScopeCenterX, hueScopeCenterY);
await page.mouse.down();
Expand All @@ -689,7 +689,7 @@ describe("calcite-color-picker", () => {
[hueScopeX] = await getElementXY(page, "calcite-color-picker", `.${CSS.hueScope}`);
[hueScopeCenterX] = getScopeCenter(hueScopeX, hueScopeY);

expect(hueScopeCenterX).toBe(hueSliderX + DIMENSIONS.m.slider.width - 1);
expect(hueScopeCenterX).toBe(hueSliderX + DIMENSIONS.m.slider.width - DIMENSIONS.m.thumb.radius);
});

describe("unsupported value handling", () => {
Expand Down Expand Up @@ -2197,23 +2197,23 @@ describe("calcite-color-picker", () => {

const getScopeLeftOffset = async () => parseFloat((await scope.getComputedStyle()).left);

expect(await getScopeLeftOffset()).toBe(-0.5);
expect(await getScopeLeftOffset()).toBeCloseTo(DIMENSIONS.m.thumb.radius - 0.5, 0);

await nudgeAQuarterOfSlider();
expect(await getScopeLeftOffset()).toBe(67.5);
expect(await getScopeLeftOffset()).toBeCloseTo(71, 0);

await nudgeAQuarterOfSlider();
expect(await getScopeLeftOffset()).toBe(135.5);
expect(await getScopeLeftOffset()).toBeCloseTo(133, 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(203.5);
expect(await getScopeLeftOffset()).toBeGreaterThan(202.5);
expect(await getScopeLeftOffset()).toBeLessThanOrEqual(194);
expect(await getScopeLeftOffset()).toBeGreaterThan(193);

// nudge it to wrap around
await scope.press("ArrowRight");
expect(await getScopeLeftOffset()).toBe(-0.5);
expect(await getScopeLeftOffset()).toBeCloseTo(DIMENSIONS.m.thumb.radius - 0.5, 0);
});

it("allows editing hue slider via keyboard", async () => {
Expand Down Expand Up @@ -2245,84 +2245,89 @@ describe("calcite-color-picker", () => {

expect(await hueSliderScope.getComputedStyle()).toMatchObject({
top: "9.5px",
left: "-0.5px",
left: `${DIMENSIONS.m.thumb.radius - 0.5}px`,
});
});
});
describe("mouse", () => {
const scopeSizeOffset = 0.8;
it("should update value when color field scope is moved", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-color-picker ></calcite-color-picker>`);
const colorPicker = await page.find("calcite-color-picker");

const [colorFieldScopeX, colorFieldScopeY] = await getElementXY(
page,
"calcite-color-picker",
`.${CSS.colorFieldScope}`
);
const value = await colorPicker.getProperty("value");
describe("mouse", () => {
const scopeSizeOffset = 0.8;
it("should update value when color field scope is moved", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-color-picker ></calcite-color-picker>`);
const colorPicker = await page.find("calcite-color-picker");

const [colorFieldScopeX, colorFieldScopeY] = await getElementXY(
page,
"calcite-color-picker",
`.${CSS.colorFieldScope}`
);
const value = await colorPicker.getProperty("value");

await page.mouse.move(colorFieldScopeX, colorFieldScopeY + scopeSizeOffset);
await page.mouse.down();
await page.mouse.up();
await page.waitForChanges();
expect(await colorPicker.getProperty("value")).not.toBe(value);
});
await page.mouse.move(colorFieldScopeX, colorFieldScopeY + scopeSizeOffset);
await page.mouse.down();
await page.mouse.up();
await page.waitForChanges();
expect(await colorPicker.getProperty("value")).not.toBe(value);
});

it("should update value when hue scope is moved", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-color-picker ></calcite-color-picker>`);
const colorPicker = await page.find("calcite-color-picker");
it("should update value when hue scope is moved", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-color-picker></calcite-color-picker>`);
const colorPicker = await page.find("calcite-color-picker");

const [hueScopeX, hueScopeY] = await getElementXY(page, "calcite-color-picker", `.${CSS.hueScope}`);
const value = await colorPicker.getProperty("value");
const [hueScopeX, hueScopeY] = await getElementXY(page, "calcite-color-picker", `.${CSS.hueScope}`);
const value = await colorPicker.getProperty("value");

await page.mouse.move(hueScopeX + scopeSizeOffset, hueScopeY);
await page.mouse.down();
await page.mouse.up();
await page.waitForChanges();
expect(await colorPicker.getProperty("value")).not.toBe(value);
});
await page.mouse.move(hueScopeX + scopeSizeOffset, hueScopeY);
await page.mouse.down();
await page.mouse.up();
await page.waitForChanges();
expect(await colorPicker.getProperty("value")).not.toBe(value);
});

it("should update value when opacity scope is moved", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-color-picker alpha-channel></calcite-color-picker>`);
const [opacityScopeX, opacityScopeY] = await getElementXY(page, "calcite-color-picker", `.${CSS.opacityScope}`);
const colorPicker = await page.find("calcite-color-picker");
const value = await colorPicker.getProperty("value");

await page.mouse.move(opacityScopeX - 2, opacityScopeY);
await page.mouse.down();
await page.mouse.up();
await page.waitForChanges();
expect(await colorPicker.getProperty("value")).not.toBe(value);
it("should update value when opacity scope is moved", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-color-picker alpha-channel></calcite-color-picker>`);
const [opacityScopeX, opacityScopeY] = await getElementXY(
page,
"calcite-color-picker",
`.${CSS.opacityScope}`
);
const colorPicker = await page.find("calcite-color-picker");
const value = await colorPicker.getProperty("value");

await page.mouse.move(opacityScopeX - 2, opacityScopeY);
await page.mouse.down();
await page.mouse.up();
await page.waitForChanges();
expect(await colorPicker.getProperty("value")).not.toBe(value);
});
});
});

describe("alpha channel", () => {
it("allows editing alpha value via keyboard", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-color-picker alpha-channel value="#ffffffff"></calcite-color-picker>`);
describe("alpha channel", () => {
it("allows editing alpha value via keyboard", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-color-picker alpha-channel value="#ffffffff"></calcite-color-picker>`);

const picker = await page.find("calcite-color-picker");
const scope = await page.find(`calcite-color-picker >>> .${CSS.opacityScope}`);
const picker = await page.find("calcite-color-picker");
const scope = await page.find(`calcite-color-picker >>> .${CSS.opacityScope}`);

await scope.press("ArrowDown");
expect(await picker.getProperty("value")).toBe("#fffffffc");
await scope.press("ArrowDown");
expect(await picker.getProperty("value")).toBe("#fffffffa");
await scope.press("ArrowDown");
expect(await picker.getProperty("value")).toBe("#fffffff7");
await scope.press("ArrowDown");
expect(await picker.getProperty("value")).toBe("#fffffffc");
await scope.press("ArrowDown");
expect(await picker.getProperty("value")).toBe("#fffffffa");
await scope.press("ArrowDown");
expect(await picker.getProperty("value")).toBe("#fffffff7");

await scope.press("ArrowUp");
expect(await picker.getProperty("value")).toBe("#fffffffa");
await scope.press("ArrowUp");
expect(await picker.getProperty("value")).toBe("#fffffffa");

await scope.press("ArrowRight");
expect(await picker.getProperty("value")).toBe("#fffffffc");
await scope.press("ArrowRight");
expect(await picker.getProperty("value")).toBe("#fffffffc");

await scope.press("ArrowLeft");
expect(await picker.getProperty("value")).toBe("#fffffffa");
await scope.press("ArrowLeft");
expect(await picker.getProperty("value")).toBe("#fffffffa");
});
});
});
});
Expand Down
29 changes: 19 additions & 10 deletions packages/calcite-components/src/components/color-picker/color-picker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ import {
LocalizedComponent,
NumberingSystem,
} from "../../utils/locale";
import { clamp } from "../../utils/math";
import { clamp, remap } from "../../utils/math";
import {
connectMessages,
disconnectMessages,
Expand Down Expand Up @@ -619,7 +619,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) {
Expand Down Expand Up @@ -1095,7 +1095,7 @@ export class ColorPicker
slider: { width },
},
} = this;
const hue = (360 / width) * x;
const hue = ((HSV_LIMITS.h - 1) / width) * x;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: can we assign 1 to a self explanatory constant since its is used in other places or have a property which stores HSV_LIMITS.h -1


this.internalColorSet(this.baseColorFieldColor.hue(hue), false);
}
Expand Down Expand Up @@ -1385,7 +1385,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);
Expand Down Expand Up @@ -1417,14 +1416,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 {
Expand All @@ -1441,7 +1441,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;
Expand Down Expand Up @@ -1565,12 +1573,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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will cause the thumb to not center on pointerup event. sliderBoundX is remapped on to a new range which is adjusting the thumb to move away from the center. Applies to opacity slider also.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not following. Both thumbs and scopes are centered. 🤔

One thing to note is that the scopes will look misaligned in non-HD screens (this is currently how it's rendering on @next). Is this what you're observing?

HD

Screenshot 2023-08-02 at 1 28 27 PM
Screenshot 2023-08-02 at 1 28 31 PM
Screenshot 2023-08-02 at 1 28 34 PM

Standard

Screenshot 2023-08-02 at 1 29 10 PM
Screenshot 2023-08-02 at 1 29 14 PM
Screenshot 2023-08-02 at 1 29 17 PM

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed w/ @anveshmekala that there's an issue when sampling colors individually vs sliding. Thanks for clarifying!

I'll look into only mapping if the x lies within the slider edges.


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 => {
Expand Down
10 changes: 9 additions & 1 deletion packages/calcite-components/src/utils/math.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { clamp, decimalPlaces } from "./math";
import { clamp, decimalPlaces, remap } from "./math";

describe("clamp", () => {
it("clamps numbers within min/max", () => {
Expand All @@ -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);
});
});
4 changes: 4 additions & 0 deletions packages/calcite-components/src/utils/math.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}