Skip to content

feat(color-picker): replaces thumb focus outline to rounded #7378

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

Merged
anveshmekala merged 27 commits into from
Aug 1, 2023
Merged

feat(color-picker): replaces thumb focus outline to rounded #7378

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
29fa91d
feat(color-picker): replaces thumb focus outline to rounded
anveshmekala Jul 25, 2023
6cd2fff
use integer values for width and height
anveshmekala Jul 25, 2023
d63ba1b
fix test errors
anveshmekala Jul 25, 2023
6aefbc4
add listener for pointerdown event from thumb
anveshmekala Jul 26, 2023
4fe5813
remove console.log statements
anveshmekala Jul 26, 2023
238bfea
refactor
anveshmekala Jul 26, 2023
7131ed9
replace css with tailwind util classes
anveshmekala Jul 26, 2023
8ae809d
add spec test
anveshmekala Jul 27, 2023
f66d93d
cleanup
anveshmekala Jul 27, 2023
ce98213
Merge branch 'main' into anveshmekala/4633-color-picker-fix-focus-out…
anveshmekala Jul 27, 2023
2af2d33
add more tests and feedback changes
anveshmekala Jul 27, 2023
46cd9f6
refactor
anveshmekala Jul 28, 2023
4610739
rename
anveshmekala Jul 28, 2023
9ea0443
remove new lines
anveshmekala Jul 28, 2023
3b98bc1
more cleanup
anveshmekala Jul 28, 2023
cb33490
disable tab container
anveshmekala Jul 28, 2023
57e0d3c
revert tab focus changes
anveshmekala Jul 28, 2023
355c0ae
Merge branch 'main' into anveshmekala/4633-color-picker-fix-focus-out…
anveshmekala Jul 28, 2023
8ce2e6d
remove empty new lines
anveshmekala Jul 31, 2023
858fafa
change test util method name
anveshmekala Jul 31, 2023
3c98108
add focus screenshot test
anveshmekala Jul 31, 2023
3d585ff
remove pointerDown handler for scope node
anveshmekala Jul 31, 2023
d1bc943
update css
anveshmekala Jul 31, 2023
193c54d
feedback changes
anveshmekala Jul 31, 2023
51c728c
change outline offset
anveshmekala Aug 1, 2023
17102cd
tidy up
anveshmekala Aug 1, 2023
6488636
increase delay for focus test
anveshmekala Aug 1, 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
251 changes: 155 additions & 96 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
@@ -1,5 +1,5 @@
import { accessible, defaults, hidden, reflects, renders, focusable, disabled, t9n } from "../../tests/commonTests";
import { CSS, DEFAULT_COLOR, DEFAULT_STORAGE_KEY_PREFIX, DIMENSIONS } from "./resources";
import { CSS, DEFAULT_COLOR, DEFAULT_STORAGE_KEY_PREFIX, DIMENSIONS, SCOPE_SIZE } from "./resources";
import { E2EElement, E2EPage, EventSpy, newE2EPage } from "@stencil/core/testing";
import { ColorValue } from "./interfaces";
import SpyInstance = jest.SpyInstance;
Expand All @@ -22,6 +22,10 @@ describe("calcite-color-picker", () => {
await page.waitForChanges();
}

function getScopeCenter(X: number, Y: number): [number, number] {
Copy link
Copy Markdown
Member

@jcfranco jcfranco Jul 31, 2023

Choose a reason for hiding this comment

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

✨🏆✨

return [X + SCOPE_SIZE / 2, Y + SCOPE_SIZE / 2];
}

beforeEach(
() =>
(consoleSpy = jest.spyOn(console, "warn").mockImplementation(() => {
Expand Down Expand Up @@ -663,26 +667,29 @@ describe("calcite-color-picker", () => {
const [hueSliderX] = await getElementXY(page, "calcite-color-picker", `.${CSS.hueSlider}`);

let [hueScopeX, hueScopeY] = await getElementXY(page, "calcite-color-picker", `.${CSS.hueScope}`);
let [hueScopeCenterX, hueScopeCenterY] = getScopeCenter(hueScopeX, hueScopeY);

await page.mouse.move(hueScopeX, hueScopeY);
await page.mouse.move(hueScopeCenterX, hueScopeCenterY);
await page.mouse.down();
await page.mouse.move(0, hueScopeY);
await page.mouse.move(0, hueScopeCenterY);
await page.mouse.up();
await page.waitForChanges();

[hueScopeX, hueScopeY] = await getElementXY(page, "calcite-color-picker", `.${CSS.hueScope}`);
[hueScopeCenterX, hueScopeCenterY] = getScopeCenter(hueScopeX, hueScopeY);

expect(hueScopeX).toBe(hueSliderX);
expect(hueScopeCenterX).toBe(hueSliderX);

await page.mouse.move(hueScopeX, hueScopeY);
await page.mouse.move(hueScopeCenterX, hueScopeCenterY);
await page.mouse.down();
await page.mouse.move(hueScopeX + DIMENSIONS.m.slider.width, hueScopeY);
await page.mouse.move(hueScopeCenterX + DIMENSIONS.m.slider.width, hueScopeCenterY);
await page.mouse.up();
await page.waitForChanges();

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

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

describe("unsupported value handling", () => {
Expand Down Expand Up @@ -2127,117 +2134,169 @@ describe("calcite-color-picker", () => {
await assertHiddenSection([]);
});

describe("scope keyboard interaction", () => {
it("allows editing color field via keyboard", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-color-picker allow-empty value=""></calcite-color-picker>`);
describe("scope interaction", () => {
describe("keyboard", () => {
it("allows editing color field via keyboard", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-color-picker allow-empty value=""></calcite-color-picker>`);

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

await scope.press("Tab");
expect(await picker.getProperty("value")).toBeFalsy();
await scope.press("ArrowDown");
expect(await picker.getProperty("value")).toBe("#ffffff");
await scope.press("ArrowDown");
expect(await picker.getProperty("value")).toBe("#ededed");
await scope.press("ArrowDown");
expect(await picker.getProperty("value")).toBe("#dbdbdb");
await scope.press("ArrowUp");
expect(await picker.getProperty("value")).toBe("#ededed");
await scope.press("ArrowRight");
expect(await picker.getProperty("value")).toBe("#e4eaed");
await scope.press("ArrowLeft");
expect(await picker.getProperty("value")).toBe("#ededed");
});
const picker = await page.find("calcite-color-picker");
const scope = await page.find(`calcite-color-picker >>> .${CSS.colorFieldScope}`);

await scope.press("Tab");
expect(await picker.getProperty("value")).toBeFalsy();
await scope.press("ArrowDown");
expect(await picker.getProperty("value")).toBe("#ffffff");
await scope.press("ArrowDown");
expect(await picker.getProperty("value")).toBe("#ededed");
await scope.press("ArrowDown");
expect(await picker.getProperty("value")).toBe("#dbdbdb");
await scope.press("ArrowUp");
expect(await picker.getProperty("value")).toBe("#ededed");
await scope.press("ArrowRight");
expect(await picker.getProperty("value")).toBe("#e4eaed");
await scope.press("ArrowLeft");
expect(await picker.getProperty("value")).toBe("#ededed");
});

it("allows nudging color's saturation even if it does not change RGB value", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-color-picker value="#000"></calcite-color-picker>`);
const scope = await page.find(`calcite-color-picker >>> .${CSS.colorFieldScope}`);
it("allows nudging color's saturation even if it does not change RGB value", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-color-picker value="#000"></calcite-color-picker>`);
const scope = await page.find(`calcite-color-picker >>> .${CSS.colorFieldScope}`);

const initialStyle = await scope.getComputedStyle();
expect(initialStyle.left).toBe("0px");
const initialStyle = await scope.getComputedStyle();
expect(initialStyle.left).toBe("-0.5px");

await clickScope(page, "color-field");
await clickScope(page, "color-field");

let nudgesToTheEdge = 25;
let nudgesToTheEdge = 25;

while (nudgesToTheEdge--) {
await scope.press("ArrowRight");
}
await page.waitForChanges();
while (nudgesToTheEdge--) {
await scope.press("ArrowRight");
}
await page.waitForChanges();

const finalStyle = await scope.getComputedStyle();
expect(finalStyle.left).toBe(`${DIMENSIONS.m.colorField.width}px`);
});
const finalStyle = await scope.getComputedStyle();
expect(finalStyle.left).toBe(`${DIMENSIONS.m.colorField.width - SCOPE_SIZE / 2}px`);
});

it("allows nudging color's hue even if it does not change RGB value", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-color-picker value="#000"></calcite-color-picker>`);
const scope = await page.find(`calcite-color-picker >>> .${CSS.hueScope}`);
it("allows nudging color's hue even if it does not change RGB value", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-color-picker value="#000"></calcite-color-picker>`);
const scope = await page.find(`calcite-color-picker >>> .${CSS.hueScope}`);

const nudgeAQuarterOfSlider = async () => {
let totalNudgesByTen = 12;
const nudgeAQuarterOfSlider = async () => {
let totalNudgesByTen = 12;

while (totalNudgesByTen--) {
await page.keyboard.down("Shift");
await scope.press("ArrowRight");
await page.keyboard.up("Shift");
}
};
while (totalNudgesByTen--) {
await page.keyboard.down("Shift");
await scope.press("ArrowRight");
await page.keyboard.up("Shift");
}
};

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

expect(await getScopeLeftOffset()).toBe(0);
expect(await getScopeLeftOffset()).toBe(-0.5);

await nudgeAQuarterOfSlider();
expect(await getScopeLeftOffset()).toBe(68);
await nudgeAQuarterOfSlider();
expect(await getScopeLeftOffset()).toBe(67.5);

await nudgeAQuarterOfSlider();
expect(await getScopeLeftOffset()).toBe(136);
await nudgeAQuarterOfSlider();
expect(await getScopeLeftOffset()).toBe(135.5);

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);
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);

// nudge it to wrap around
await scope.press("ArrowRight");
expect(await getScopeLeftOffset()).toBe(0);
});
// nudge it to wrap around
await scope.press("ArrowRight");
expect(await getScopeLeftOffset()).toBe(-0.5);
});

it("allows editing hue slider via keyboard", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-color-picker allow-empty value=""></calcite-color-picker>`);
it("allows editing hue slider via keyboard", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-color-picker allow-empty value=""></calcite-color-picker>`);

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

await hueScope.press("ArrowDown");
expect(await picker.getProperty("value")).toBe("#007ec2");
await hueScope.press("ArrowRight");
expect(await picker.getProperty("value")).toBe("#007bc2");
await hueScope.press("ArrowLeft");
expect(await picker.getProperty("value")).toBe("#007ec2");

await page.keyboard.press("Shift");
await hueScope.press("ArrowDown");
expect(await picker.getProperty("value")).toBe("#0081c2");
await hueScope.press("ArrowUp");
expect(await picker.getProperty("value")).toBe("#007ec2");
const picker = await page.find("calcite-color-picker");
const hueScope = await page.find(`calcite-color-picker >>> .${CSS.hueScope}`);

await hueScope.press("ArrowDown");
expect(await picker.getProperty("value")).toBe("#007ec2");
await hueScope.press("ArrowRight");
expect(await picker.getProperty("value")).toBe("#007bc2");
await hueScope.press("ArrowLeft");
expect(await picker.getProperty("value")).toBe("#007ec2");

await page.keyboard.press("Shift");
await hueScope.press("ArrowDown");
expect(await picker.getProperty("value")).toBe("#0081c2");
await hueScope.press("ArrowUp");
expect(await picker.getProperty("value")).toBe("#007ec2");
});

it("positions the scope correctly when the color is 000", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-color-picker value="#000"></calcite-color-picker>`);

const hueSliderScope = await page.find(`calcite-color-picker >>> .${CSS.hueScope}`);

expect(await hueSliderScope.getComputedStyle()).toMatchObject({
top: "9.5px",
left: "-0.5px",
});
});
});
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");

it("positions the scope correctly when the color is 000", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-color-picker value="#000"></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);
});

const hueSliderScope = await page.find(`calcite-color-picker >>> .${CSS.hueScope}`);
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");

expect(await hueSliderScope.getComputedStyle()).toMatchObject({
top: "10px",
left: "0px",
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);
});
});
});
Expand Down
10 changes: 7 additions & 3 deletions packages/calcite-components/src/components/color-picker/color-picker.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,11 +78,15 @@
@apply text-n1
focus-base
absolute
z-default;

z-default
rounded-full
bg-transparent
w-px
h-px
pointer-events-none;
&:focus {
@apply focus-outset;
outline-offset: 16px;
Copy link
Copy Markdown
Member

@jcfranco jcfranco Jul 27, 2023

Choose a reason for hiding this comment

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

One more thing I noticed. The design spec seems to have a 2px space between the thumb and the focus outline. The spacing in the PR screenshot looks larger than that. Maybe adjusting this to 14px can help better match that.

Copy link
Copy Markdown
Contributor Author

@anveshmekala anveshmekala Jul 28, 2023

Choose a reason for hiding this comment

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

if we increase the spacing from 12px to 14px it will increase the gap between the thumb and outline.

Thoughts @ashetland

Copy link
Copy Markdown
Contributor Author

@anveshmekala anveshmekala Jul 31, 2023

Choose a reason for hiding this comment

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

Adding @macandcheese.

Copy link
Copy Markdown
Contributor

@macandcheese macandcheese Jul 31, 2023

Choose a reason for hiding this comment

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

If I’m following, I agree it should be a 2px gap to align with other “outset focus” styles.

outline-offset: 11px;
}
}

Expand Down
25 changes: 19 additions & 6 deletions packages/calcite-components/src/components/color-picker/color-picker.stories.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,14 +103,27 @@ export const thumbsOnEdgeDoNotOverflowContainer_TestOnly = (): string => html`<d
<calcite-color-picker value="#04006e"></calcite-color-picker>
</div>`;

export const ArabicLocale_TestOnly = (): string => html` <calcite-color-picker lang="ar"></calcite-color-picker> `;
export const ArabicLocale_TestOnly = (): string => html`<calcite-color-picker lang="ar"></calcite-color-picker>`;

export const NorwegianLocale_TestOnly = (): string => html` <calcite-color-picker lang="no"></calcite-color-picker> `;
export const NorwegianLocale_TestOnly = (): string => html`<calcite-color-picker lang="no"></calcite-color-picker>`;

export const SpanishLocale_TestOnly = (): string => html` <calcite-color-picker lang="es"></calcite-color-picker> `;
export const SpanishLocale_TestOnly = (): string => html`<calcite-color-picker lang="es"></calcite-color-picker>`;

export const JapaneseLocale_TestOnly = (): string => html` <calcite-color-picker lang="ja"></calcite-color-picker> `;
export const JapaneseLocale_TestOnly = (): string => html`<calcite-color-picker lang="ja"></calcite-color-picker>`;

export const RussianLocale_TestOnly = (): string => html` <calcite-color-picker lang="ru"></calcite-color-picker> `;
export const RussianLocale_TestOnly = (): string => html`<calcite-color-picker lang="ru"></calcite-color-picker>`;

export const ThaiLocale_TestOnly = (): string => html` <calcite-color-picker lang="th"></calcite-color-picker> `;
export const ThaiLocale_TestOnly = (): string => html`<calcite-color-picker lang="th"></calcite-color-picker>`;

export const Focus_TestOnly = (): string => html`<calcite-color-picker value="#97a7b0"></calcite-color-picker>
<script>
(async () => {
await customElements.whenDefined("calcite-color-picker");
const colorPicker = await document.querySelector("calcite-color-picker").componentOnReady();
await colorPicker.setFocus();
})();
</script>`;

Focus_TestOnly.parameters = {
chromatic: { delay: 2000 },
};
Loading