-
Notifications
You must be signed in to change notification settings - Fork 229
feat(reactive-controllers): Migrate to Colorjs from Tinycolor #4713
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.
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
Changes from all commits
cfeaf31
b9bb98a
b68e457
6b6528f
882166c
c914192
39e026f
b73eba7
f54193c
92a16f7
8034f31
a89238d
819f259
88c555b
538e82f
b5f307e
a7d5257
c39f021
6ffc250
e7d473f
2c9e7b1
17e3ab1
a81399a
3e5e030
f0b1057
f24a330
d4ef73c
5b00e07
818b100
207e432
3a8ad6e
6eb2db3
8b24244
f0c92a8
f926af0
03b3250
e4814bc
473f9a8
73d3cbe
d017a8c
ecdf380
a08cb8d
e73d438
e6fe91a
c916446
982f747
17e14b4
97885ac
ee02a20
cc9371e
0ed543f
1cf4abd
91db96a
75573ed
c7e05d6
28e48a9
72a3366
d63fe9a
1304483
ec3552b
4e21cad
36f6761
b9865b5
ce0c7a1
8e94b88
e0f63bd
dfc1b57
05ede47
584e0a1
9477aaf
cae4cfb
8925192
442fa60
e65242c
1e65894
db6c3bc
91a8226
fb75701
7293bcb
ff0885b
7fbba2f
d4180cc
2081341
2bbeb2b
c1b9a08
43dc2c2
a908293
9ebc7ed
a1ca107
719c4f3
8d443d4
ef73ff4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,10 +17,7 @@ import { | |
SpectrumElement, | ||
TemplateResult, | ||
} from '@spectrum-web-components/base'; | ||
import { | ||
ifDefined, | ||
styleMap, | ||
} from '@spectrum-web-components/base/src/directives.js'; | ||
import { ifDefined } from '@spectrum-web-components/base/src/directives.js'; | ||
import { | ||
property, | ||
query, | ||
|
@@ -29,10 +26,11 @@ import { streamingListener } from '@spectrum-web-components/base/src/streaming-l | |
import { SWCResizeObserverEntry, WithSWCResizeObserver } from './types'; | ||
import type { ColorHandle } from '@spectrum-web-components/color-handle'; | ||
import '@spectrum-web-components/color-handle/sp-color-handle.js'; | ||
|
||
import { | ||
ColorController, | ||
ColorValue, | ||
} from '@spectrum-web-components/reactive-controllers/src/Color.js'; | ||
ColorTypes, | ||
} from '@spectrum-web-components/reactive-controllers/src/ColorController.js'; | ||
import { LanguageResolutionController } from '@spectrum-web-components/reactive-controllers/src/LanguageResolution.js'; | ||
import { | ||
isAndroid, | ||
|
@@ -72,18 +70,35 @@ export class ColorArea extends SpectrumElement { | |
|
||
private languageResolver = new LanguageResolutionController(this); | ||
|
||
private colorController = new ColorController(this, { | ||
extractColorFromState: () => ({ | ||
h: this.hue, | ||
s: this.x, | ||
v: this.y, | ||
}), | ||
applyColorToState: ({ s, v }) => { | ||
this._x = s; | ||
this._y = v; | ||
this.requestUpdate(); | ||
}, | ||
}); | ||
/** | ||
* A controller for managing color interactions within the ColorArea component. | ||
* | ||
* The `ColorController` is instantiated with the `manageAs` option set to `hsv` | ||
* because the ColorArea component is designed to manipulate the saturation (`s`) | ||
* and value (`v`) components of the HSV color model along the x and y axes, | ||
* respectively. In the HSV color model: | ||
* | ||
* - The `hue` (h) represents the color type and is typically controlled by a separate input. | ||
* - The `saturation` (s) represents the intensity of the color, ranging from 0% (gray) to 100% (full color). | ||
* - The `value` (v) represents the brightness of the color, ranging from 0% (black) to 100% (full brightness). | ||
* | ||
* In the ColorArea component: | ||
* | ||
* - The x-axis controls the saturation (`s`), allowing users to adjust the intensity of the color. | ||
* - The y-axis controls the value (`v`), allowing users to adjust the brightness of the color. | ||
* | ||
* By managing the color as `hsv`, the ColorController can efficiently handle the changes in saturation and value | ||
* as the user interacts with the ColorArea component. | ||
* | ||
* @private | ||
* @type {ColorController} | ||
* @memberof ColorArea | ||
* | ||
* @property {ColorArea} this - The instance of the ColorArea component. | ||
* @property {Object} options - Configuration options for the ColorController. | ||
* @property {string} options.manageAs - Specifies the color model to manage, in this case 'hsv'. | ||
*/ | ||
private colorController = new ColorController(this, { manageAs: 'hsv' }); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you create a documentation block here. It would be easier to understand why a object with param manageAs is needed for colorjs.io There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have added docs block in color-controller and explained the use of manage as there.
|
||
|
||
@property({ type: Number }) | ||
public get hue(): number { | ||
|
@@ -95,16 +110,16 @@ export class ColorArea extends SpectrumElement { | |
} | ||
|
||
@property({ type: String }) | ||
public get value(): ColorValue { | ||
return this.colorController.color; | ||
public get value(): ColorTypes { | ||
return this.colorController.colorValue; | ||
} | ||
|
||
@property({ type: String }) | ||
public get color(): ColorValue { | ||
return this.colorController.color; | ||
public get color(): ColorTypes { | ||
return this.colorController.colorValue; | ||
} | ||
|
||
public set color(color: ColorValue) { | ||
public set color(color: ColorTypes) { | ||
this.colorController.color = color; | ||
} | ||
|
||
|
@@ -113,48 +128,48 @@ export class ColorArea extends SpectrumElement { | |
|
||
@property({ type: Number }) | ||
public get x(): number { | ||
return this._x; | ||
return this.colorController.color.hsv.s / 100; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this be simplified? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately, this can't be more simplified because to to access the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with @Rajdeepc. And I'd like to add that anything that multiple components might use should be part of the controller. @blunteshwar what if we added a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This getX method is very specific to color-area and doesn't exist in other color components, thus creating a getter inside colorController just to cater a single component doesn't make much sense. |
||
} | ||
|
||
public set x(x: number) { | ||
if (x === this.x) { | ||
return; | ||
} | ||
const oldValue = this.x; | ||
this._x = x; | ||
if (this.inputX) { | ||
// Use the native `input[type='range']` control to validate this value after `firstUpdate` | ||
this.inputX.value = x.toString(); | ||
this._x = this.inputX.valueAsNumber; | ||
this.colorController.color.set( | ||
's', | ||
this.inputX.valueAsNumber * 100 | ||
); | ||
} else { | ||
this.colorController.color.set('s', x * 100); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see a lot of multiplier with 100. Is this something cannot be overriden out of the box? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, we are multiplying x and y coordinates by 100 to convert coordinate values into saturation and value respectively. We are doing this 4 times in the code, Twice for x coordinate and twice for y coordinates. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @blunteshwar What if we added a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again this setX is very specific to color area and doesn't exist in other color components. |
||
} | ||
this.requestUpdate('x', oldValue); | ||
this.colorController.applyColorFromState(); | ||
} | ||
|
||
private _x = 1; | ||
|
||
@property({ type: Number }) | ||
public get y(): number { | ||
return this._y; | ||
return this.colorController.color.hsv.v / 100; | ||
nikkimk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
public set y(y: number) { | ||
if (y === this.y) { | ||
return; | ||
} | ||
const oldValue = this.y; | ||
this._y = y; | ||
if (this.inputY) { | ||
// Use the native `input[type='range']` control to validate this value after `firstUpdate` | ||
this.inputY.value = y.toString(); | ||
this._y = this.inputY.valueAsNumber; | ||
this.colorController.color.set( | ||
'v', | ||
this.inputY.valueAsNumber * 100 | ||
); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is also very specific to color-area only! |
||
} | ||
this.requestUpdate('y', oldValue); | ||
this.colorController.applyColorFromState(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removing this you are also not refreshing the update lifecycle in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We do not need to do this because the component is getting updated by |
||
} | ||
|
||
private _y = 1; | ||
|
||
@property({ type: Number }) | ||
public step = 0.01; | ||
|
||
|
@@ -201,6 +216,7 @@ export class ColorArea extends SpectrumElement { | |
private handleKeydown(event: KeyboardEvent): void { | ||
const { code } = event; | ||
this.focused = true; | ||
|
||
this.altered = [event.shiftKey, event.ctrlKey, event.altKey].filter( | ||
(key) => !!key | ||
).length; | ||
|
@@ -262,7 +278,6 @@ export class ColorArea extends SpectrumElement { | |
this.y = Math.min(1, Math.max(this.y + deltaY, 0)); | ||
|
||
this.colorController.savePreviousColor(); | ||
this.colorController.applyColorFromState(); | ||
|
||
if (deltaX != 0 || deltaY != 0) { | ||
this._valueChanged = true; | ||
|
@@ -295,7 +310,6 @@ export class ColorArea extends SpectrumElement { | |
const { valueAsNumber, name } = event.target; | ||
|
||
this[name as 'x' | 'y'] = valueAsNumber; | ||
this.colorController.applyColorFromState(); | ||
} | ||
|
||
private handleChange(event: Event & { target: HTMLInputElement }): void { | ||
|
@@ -332,8 +346,8 @@ export class ColorArea extends SpectrumElement { | |
this._valueChanged = false; | ||
|
||
this.x = x; | ||
this.y = 1 - y; | ||
this.colorController.applyColorFromState(); | ||
this.y = y; | ||
|
||
this.dispatchEvent( | ||
new Event('input', { | ||
bubbles: true, | ||
|
@@ -390,7 +404,7 @@ export class ColorArea extends SpectrumElement { | |
Math.min(1, (offsetY - minOffsetY) / height) | ||
); | ||
|
||
return [this.isLTR ? percentX : 1 - percentX, percentY]; | ||
return [this.isLTR ? percentX : 1 - percentX, 1 - percentY]; | ||
} | ||
|
||
private handleAreaPointerdown(event: PointerEvent): void { | ||
|
@@ -438,7 +452,7 @@ export class ColorArea extends SpectrumElement { | |
<div | ||
@pointerdown=${this.handleAreaPointerdown} | ||
class="gradient" | ||
style=${styleMap(style)} | ||
style="background: ${style.background};" | ||
> | ||
<slot name="gradient"></slot> | ||
</div> | ||
|
@@ -534,13 +548,31 @@ export class ColorArea extends SpectrumElement { | |
this.addEventListener('keydown', this.handleKeydown); | ||
} | ||
|
||
/** | ||
* Overrides the `updated` method to handle changes in property values. | ||
* | ||
* @param changed - A map of changed properties with their previous values. | ||
* | ||
* This method performs the following actions: | ||
* - Updates the saturation (`s`) of the color if `x` has changed. | ||
* - Updates the value (`v`) of the color if `y` has changed. | ||
* - If the `focused` property has changed and is now true, it lazily binds | ||
* the `input[type="range"]` elements in shadow roots to prevent multiple | ||
* tab stops within the Color Area for certain browser settings (e.g., Webkit). | ||
*/ | ||
protected override updated(changed: PropertyValues): void { | ||
super.updated(changed); | ||
if (this.x !== this.inputX.valueAsNumber) { | ||
this._x = this.inputX.valueAsNumber; | ||
this.colorController.color.set( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please create a documentation block here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! |
||
's', | ||
this.inputX.valueAsNumber * 100 | ||
); | ||
} | ||
if (this.y !== this.inputY.valueAsNumber) { | ||
this._y = this.inputY.valueAsNumber; | ||
this.colorController.color.set( | ||
'v', | ||
(1 - this.inputY.valueAsNumber) * 100 | ||
); | ||
} | ||
if (changed.has('focused') && this.focused) { | ||
// Lazily bind the `input[type="range"]` elements in shadow roots | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way we can re-use the above two functions? It makes the code more cleaner and abstracted rather than manipulating a nested object like
this.colorController.color.hsv.s
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are using
this.colorController.color.hsv.s
only once and there too we are not manipulating the value of this nested object we are just returning it.This is very specific to color-area hence creating a getter inside color-controller doesn't make much sense