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
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 4 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
31 changes: 17 additions & 14 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 @@ -6,6 +6,8 @@ import SpyInstance = jest.SpyInstance;
import { GlobalTestProps, selectText, getElementXY, newProgrammaticE2EPage } from "../../tests/utils";
import { html } from "../../../support/formatting";

const scopeNodeOffset = DIMENSIONS.scopeNode / 2;

describe("calcite-color-picker", () => {
let consoleSpy: SpyInstance;

Expand Down Expand Up @@ -659,20 +661,21 @@ describe("calcite-color-picker", () => {

it("does not wrap the hue slider thumb when dragging past the edge", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-color-picker></calcite-color-picker>`);
await page.setContent(html`<calcite-color-picker></calcite-color-picker>`);

const [hueSliderX] = await getElementXY(page, "calcite-color-picker", `.${CSS.hueSlider}`);

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

await page.mouse.move(hueScopeX, hueScopeY);
await page.mouse.move(hueScopeX + scopeNodeOffset, hueScopeY + scopeNodeOffset);
await page.mouse.down();
await page.mouse.move(0, hueScopeY);
await page.mouse.move(0, hueScopeY + scopeNodeOffset);
await page.mouse.up();
await page.waitForChanges();

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

expect(hueScopeX).toBe(hueSliderX);
expect(hueScopeX).toBe(hueSliderX - scopeNodeOffset);

await page.mouse.move(hueScopeX, hueScopeY);
await page.mouse.down();
Expand Down Expand Up @@ -2157,7 +2160,7 @@ describe("calcite-color-picker", () => {
const scope = await page.find(`calcite-color-picker >>> .${CSS.colorFieldScope}`);

const initialStyle = await scope.getComputedStyle();
expect(initialStyle.left).toBe("0px");
expect(initialStyle.left).toBe(`${0 - scopeNodeOffset}px`);

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

Expand All @@ -2169,7 +2172,7 @@ describe("calcite-color-picker", () => {
await page.waitForChanges();

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

it("allows nudging color's hue even if it does not change RGB value", async () => {
Expand All @@ -2189,23 +2192,23 @@ describe("calcite-color-picker", () => {

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

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

await nudgeAQuarterOfSlider();
expect(await getScopeLeftOffset()).toBe(68);
expect(await getScopeLeftOffset()).toBe(68 - scopeNodeOffset);

await nudgeAQuarterOfSlider();
expect(await getScopeLeftOffset()).toBe(136);
expect(await getScopeLeftOffset()).toBe(136 - scopeNodeOffset);

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(204 - scopeNodeOffset);
expect(await getScopeLeftOffset()).toBeGreaterThan(203 - scopeNodeOffset);

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

it("allows editing hue slider via keyboard", async () => {
Expand Down Expand Up @@ -2236,8 +2239,8 @@ describe("calcite-color-picker", () => {
const hueSliderScope = await page.find(`calcite-color-picker >>> .${CSS.hueScope}`);

expect(await hueSliderScope.getComputedStyle()).toMatchObject({
top: "10px",
left: "0px",
top: `${10 - scopeNodeOffset}px`,
left: `${0 - scopeNodeOffset}px`,
});
});
});
Expand Down
9 changes: 6 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,14 @@
@apply text-n1
focus-base
absolute
z-default;

z-default
rounded-full
bg-transparent;
inline-size: 1px;
block-size: 1px;
&:focus {
@apply focus-outset;
outline-offset: 16px;
Copy link
Member

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
Contributor Author

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
Contributor Author

Choose a reason for hiding this comment

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

Adding @macandcheese.

Copy link
Contributor

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: 12px;
}
}

Expand Down
22 changes: 20 additions & 2 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 @@ -539,6 +539,11 @@ export class ColorPicker
context: this.colorFieldRenderingContext,
bounds: this.colorFieldRenderingContext.canvas.getBoundingClientRect(),
};

if (event.target === this.colorFieldScopeNode) {
return;
}

this.captureColorFieldColor(offsetX, offsetY);
this.colorFieldScopeNode.focus();
};
Expand All @@ -557,6 +562,9 @@ export class ColorPicker
context: this.hueSliderRenderingContext,
bounds: this.hueSliderRenderingContext.canvas.getBoundingClientRect(),
};
if (event.target === this.hueScopeNode) {
return;
}
this.captureHueSliderColor(offsetX);
this.hueScopeNode.focus();
};
Expand All @@ -580,6 +588,7 @@ export class ColorPicker
};

private globalPointerUpHandler = (event: PointerEvent): void => {
console.log("pointer up");
if (!isPrimaryPointerButton(event)) {
return;
}
Expand All @@ -597,6 +606,7 @@ export class ColorPicker
const { activeCanvasInfo, el } = this;

if (!el.isConnected || !activeCanvasInfo) {
console.log("move handler");
return;
}

Expand Down Expand Up @@ -769,8 +779,12 @@ export class ColorPicker
aria-valuenow={(vertical ? color?.saturationv() : color?.value()) || "0"}
class={{ [CSS.scope]: true, [CSS.colorFieldScope]: true }}
onKeyDown={this.handleColorFieldScopeKeyDown}
onPointerDown={this.handleColorFieldPointerDown}
role="slider"
style={{ top: `${colorFieldScopeTop || 0}px`, left: `${colorFieldScopeLeft || 0}px` }}
style={{
top: `${colorFieldScopeTop - DIMENSIONS.scopeNode / 2 || 0}px`,
left: `${colorFieldScopeLeft - DIMENSIONS.scopeNode / 2 || 0}px`,
}}
tabindex="0"
// eslint-disable-next-line react/jsx-sort-props
ref={this.storeColorFieldScope}
Expand All @@ -793,8 +807,12 @@ export class ColorPicker
aria-valuenow={color?.round().hue() || DEFAULT_COLOR.round().hue()}
class={{ [CSS.scope]: true, [CSS.hueScope]: true }}
onKeyDown={this.handleHueScopeKeyDown}
onPointerDown={this.handleHueSliderPointerDown}
role="slider"
style={{ top: `${hueTop}px`, left: `${hueLeft}px` }}
style={{
top: `${hueTop - DIMENSIONS.scopeNode / 2}px`,
Copy link
Member

Choose a reason for hiding this comment

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

since a few places are dividing a value by the scopeNode and converting to px maybe convert that into a function for reuse.

left: `${hueLeft - DIMENSIONS.scopeNode / 2}px`,
}}
tabindex="0"
// eslint-disable-next-line react/jsx-sort-props
ref={this.storeHueScope}
Expand Down
1 change: 1 addition & 0 deletions packages/calcite-components/src/components/color-picker/resources.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,4 +93,5 @@ export const DIMENSIONS = {
radius: 10,
},
},
scopeNode: 1,
};