feat(reactive-controllers): Migrate to Colorjs from Tinycolor #4713
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Branch previewReview the following VRT differencesWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
If the changes are expected, update the |
Pull Request Test Coverage Report for Build 11897289030Details
💛 - Coveralls |
Tachometer resultsChromeaccordion permalinkbasic-test
action-bar permalinkbasic-test
action-group permalinkbasic-test
action-menu permalinktest-basic
test-directive permalink
test-lazy permalink
test-open-close-directive permalink
test-open-close permalink
breadcrumbs permalinkbasic-test
coachmark permalinkbasic-test
color-area permalinkbasic-test
color-field permalinkbasic-test
color-slider permalinkbasic-test
color-wheel permalinkbasic-test
combobox permalinkbasic-test
light-dom-test permalink
contextual-help permalinkbasic-test
field-label permalinkbasic-test
grid permalinkbasic-test
menu permalinktest-basic
meter permalinkbasic-test
number-field permalinkbasic-test
overlay permalinkbasic-test
directive-test permalink
element-test permalink
lazy-test permalink
picker permalinkbasic-test
popover permalinktest-basic
progress-bar permalinkbasic-test
radio permalinktest-basic
sidenav permalinktest-basic
slider permalinktest-basic
swatch permalinkbasic-test
tabs permalinkbasic-test
tags permalinkbasic-test
tooltip permalinktest-basic
test-directive permalink
test-element permalink
test-lazy permalink
top-nav permalinkbasic-test
tray permalinkbasic-test
truncated permalinkbasic-test
Firefoxaccordion permalinkbasic-test
action-bar permalinkbasic-test
action-group permalinkbasic-test
action-menu permalinktest-basic
test-directive permalink
test-lazy permalink
test-open-close-directive permalink
test-open-close permalink
breadcrumbs permalinkbasic-test
coachmark permalinkbasic-test
color-area permalinkbasic-test
color-field permalinkbasic-test
color-slider permalinkbasic-test
color-wheel permalinkbasic-test
combobox permalinkbasic-test
light-dom-test permalink
contextual-help permalinkbasic-test
field-label permalinkbasic-test
grid permalinkbasic-test
menu permalinktest-basic
meter permalinkbasic-test
number-field permalinkbasic-test
overlay permalinkbasic-test
directive-test permalink
element-test permalink
lazy-test permalink
picker permalinkbasic-test
popover permalinktest-basic
progress-bar permalinkbasic-test
radio permalinktest-basic
sidenav permalinktest-basic
slider permalinktest-basic
swatch permalinkbasic-test
tabs permalinkbasic-test
tags permalinkbasic-test
tooltip permalinktest-basic
test-directive permalink
test-element permalink
test-lazy permalink
top-nav permalinkbasic-test
tray permalinkbasic-test
truncated permalinkbasic-test
|
Lighthouse scores
What is this?Lighthouse scores comparing the documentation site built from the PR ("Branch") to that of the production documentation site ("Latest") and the build currently on Transfer Size
Request Count
|
blunteshwar
changed the title
…nents into color-migration
1 task
1 task
Rajdeepc
requested changes
Nov 13, 2024
| @@ -113,48 +103,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.
Unfortunately, this can't be more simplified because to to access the s coordinate value we have to access the color via colorController. After fetching this s coordinate value we divide it by 100 to convert it into decimal from percentage and then this converted s value our x coordinate for color-area.
| this.inputX.valueAsNumber * 100 | ||
| ); | ||
| } else { | ||
| this.colorController.color.set('s', x * 100); |
There was a problem hiding this comment.
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.
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.
Thus it can't be overriden out of the box.
packages/color-area/src/ColorArea.ts
Outdated
| @@ -164,7 +154,7 @@ export class ColorArea extends SpectrumElement { | |||
| @query('[name="y"]') | |||
| public inputY!: HTMLInputElement; | |||
|
|
|||
| private altered = 0; | |||
| public altered = 0; | |||
There was a problem hiding this comment.
This can be access outside of this class! Do we need it to be like this?
There was a problem hiding this comment.
Makes sense!
Done!
| @@ -145,10 +145,15 @@ describe('ColorArea', () => { | |||
| await sendKeys({ | |||
| press: 'ArrowDown', | |||
| }); | |||
| await sendKeys({ | |||
There was a problem hiding this comment.
Why did you need to repeat it?
There was a problem hiding this comment.
This is because of the fact that hsl color space can have multiple corresponding hsv values, for instance hsv(100°, 68%, 75%) and hsv(100°, 68%, 74%) both corresponds to hsl(100°, 52%, 49%). Thus before first await sendKeys({press: 'ArrowDown',}); the value of color in hsv is hsv(100°, 68%, 75%) and when we press ArrowDown once the value of s coordinate decreases from 75 to 74 and color becomes hsv(100°, 68%, 74%) but as stated above this color value also corresponds to hsl(100°, 52%, 49%). Therefore the solution is either to press ArrowDown again test this in hsv color space instead of hsl
| el.y = 0.7; | ||
| await el.updateComplete; | ||
| expect(handle.color).to.equal('hsl(0, 100%, 35%)'); | ||
| expect(handle.color).to.equal('hsl(0 100% 35%)'); |
There was a problem hiding this comment.
If we add like this it should pass also! 'hsl(0, 100%, 35%)'
There was a problem hiding this comment.
Nope, our color controller accepts colors in a flexible formats but it outputs them in a specific std. format, either in a CSS string (using color.js native toString() method which converts native color objects into css strings) or in object format.
Since sp-color-handle does not leverage any color library and merely requires a css color string to assign itself a color value, thus sp-color-area assigns a css color string to sp-color-handle by leveraging the toString() method of color.js which returns colors in space separated format.
For more info:https://colorjs.io/docs/output
| @@ -799,7 +802,7 @@ describe('ColorArea', () => { | |||
| await elementUpdated(el); | |||
|
|
|||
| let outputColor = el.color as { h: number; s: number; l: number }; | |||
| const variance = 0.00005; | |||
| const variance = 0.004; | |||
There was a problem hiding this comment.
Why did we need to change the variance value?
There was a problem hiding this comment.
Internally sp-color-area is managed by hsv color space where for a color area h: hue value is constant and s: saturation value varies along "x" axis and v: value varies across "y" axis of sp-color-area. Now when we initialise it's color value in hsl format (line 796), the colorController internally converts it to hsv which is very very less lossy but still lossy. Then on line 804 we are again fetching the color value in hsl format so the colotController again converts the internal color object (which is in hsv) to hsl. Thus these very very minor losses are still losses due to which we have to reduce the variance value. Earlier the tests were passing because sp-color-area was not properly implemented using hsv color space.
Now we either have to reduce the variance value(which was also done by westbrook in hus pr line 705) or we should test this in hsv color-space.
| | HSL | ||
| | HSLA; | ||
| color: ColorTypes; | ||
| test?: string; |
There was a problem hiding this comment.
What is a test color Format?
There was a problem hiding this comment.
We are leveraging this test to test for a string color, for eg when color='red' we need some source of truth or quantifiable value to test for red. This quantifiable value is being stored inside test variable.(line 901 and 923)
| ? html` | ||
| <sp-color-handle | ||
| size="m" | ||
| color="${this.getColorValue()}" | ||
| color="${this.colorController |
There was a problem hiding this comment.
Create a getter method for this!
There was a problem hiding this comment.
getColor is already a getter method inside colorController.
| if (!this.valid) { | ||
| return ''; | ||
| } | ||
| return this.colorController.getColor('srgb').toString(); |
There was a problem hiding this comment.
How are you making sure that this.colorController.getColor('srgb') always returns something? Can it return undefined?
There was a problem hiding this comment.
I will add an if else condition in getColor
| @@ -47,22 +61,22 @@ describe('ColorField', () => { | |||
| // Initial value | |||
| el.value = '#ff0000'; | |||
| await elementUpdated(el); | |||
| expect(el.getColorValue()).to.equal('rgb(255, 0, 0)'); | |||
| expect(el.getColorValue()).to.equal('rgb(100% 0% 0%)'); | |||
There was a problem hiding this comment.
Users can add rgb values comma separated. This test is skipping that check! Do you feel you accomodate that also here?
There was a problem hiding this comment.
This is again due to the fact that color-field uses sp-color-handle which merely neeeds css color strings. These css color strings are provided by native toString() API from colorjs which provides color in string format (space separated instead of comma separated).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR marks the initial steps in our color library migration for Spectrum Web Components. As part of this effort, I have implemented an abstraction layer that includes the following key elements:
Color Types: Defined standardized types to ensure consistent handling of color data across components.
Transformation Functions: Created functions to handle color conversions and transformations, making it easier to work with various color formats.
This abstraction layer is designed to facilitate a smoother migration process and minimize the impact on our consumers. By isolating the color logic, we can easily adapt to future changes in the underlying color library.
Stay tuned for more details—there's much more to come! 🚀
Related issue(s)
#3950 ✅ (Solved)
#3883 ✅ (Solved)
#3655 ✅ (Solved)
#3071 ✅ (Solved)
#3058 ✅ (Solved)
Motivation and context
Types of changes
Checklist
Best practices
This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against
main.