feat(color): add color channel values and luminosity, saturation, clip functions #4366
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
straker
commented
Mar 8, 2024
| @@ -13,6 +13,10 @@ const hslRegex = /hsl\(\s*([\d.]+)(rad|turn)/; | |||
| */ | |||
| export default class Color { | |||
| constructor(red, green, blue, alpha = 1) { | |||
| if (red instanceof Color) { | |||
There was a problem hiding this comment.
Now we can create a new color from another one. Makes the new functions that create new colors much easier.
const color = new Color(0,0,0);
const color2 = new Color(color);
WilcoFiers
previously requested changes
Mar 11, 2024
lib/commons/color/color.js
Outdated
Comment on lines
164
to
169
| return new Color( | ||
| this.red / 255, | ||
| this.green / 255, | ||
| this.blue / 255, | ||
| this.alpha | ||
| ); |
There was a problem hiding this comment.
This object is broken. That's the issue I mentioned in the other PR too. Color is built to accept 0-255. A bunch of methods on this class don't work correctly if you pass it 0-1. When we're in 0-1 land we need different properties. We can do that with getter/setters on this class, or we could create a separate object or class to represent that.
Suggested change
| return new Color( | |
| this.red / 255, | |
| this.green / 255, | |
| this.blue / 255, | |
| this.alpha | |
| ); | |
| return { | |
| floatRed: this.red / 255, | |
| floatGreen: this.green / 255, | |
| floatBlue: this.blue / 255, | |
| floatAlpha: this.alpha | |
| ); |
There was a problem hiding this comment.
Decided to mimic how Colorjs.io does this by using color coordinates (mapped to r, g, b), also called color channels. I also learned that Babel supports private properties and functions. Just had to up our eslint version to allow it.
straker
changed the title
straker
commented
Mar 11, 2024
| @@ -2,7 +2,7 @@ module.exports = { | |||
| root: true, | |||
| extends: ['prettier'], | |||
| parserOptions: { | |||
| ecmaVersion: 2021 | |||
| ecmaVersion: 2023 | |||
There was a problem hiding this comment.
Allows us to use private properties and functions in classes (supported by babel).
straker
commented
Mar 11, 2024
| @@ -57,28 +129,12 @@ export default class Color { | |||
| * @instance | |||
| */ | |||
| parseString(colorString) { | |||
| // Colorjs currently does not support rad or turn angle values | |||
There was a problem hiding this comment.
Colorjs 0.5.0 now fully supports grad, rad, and turn so we don't need to do this anymore.
straker
commented
Mar 11, 2024
| * @param {number} value The value to add | ||
| * @return {Color} A new color instance | ||
| */ | ||
| #add(value) { |
There was a problem hiding this comment.
Nothing outside of the color class should add a value to the color components.
straker
changed the title
WilcoFiers
approved these changes
Mar 12, 2024
Merged
WilcoFiers
added a commit
that referenced
this pull request
Mar 25, 2024
## [4.9.0](v4.8.4...v4.9.0) (2024-03-25) ### Features - adding the wcag131 tag to the aria-hidden-body rule ([#4349](#4349)) ([dd4c3c3](dd4c3c3)), closes [#4315](#4315) - **checks:** deprecate aria-busy check ([#4356](#4356)) ([be0b555](be0b555)), closes [#4347](#4347) [#4340](#4340) - **color:** add color channel values and luminosity, saturation, clip functions ([#4366](#4366)) ([9e70199](9e70199)), closes [/github.com//pull/4365/files#r1517706612](https://github.com/dequelabs//github.com/dequelabs/axe-core/pull/4365/files/issues/r1517706612) - **i18n:** add Greek Translations ([#3836](#3836)) ([3ea9a48](3ea9a48)) - **i18n:** Add Italian translation ([#4344](#4344)) ([de1baa9](de1baa9)) - **i18n:** Add Simplified Chinese translation ([#4379](#4379)) ([bda7c8d](bda7c8d)) - **i18n:** Add Taiwanese Mandarin translation ([#4299](#4299)) ([c5e11de](c5e11de)) ### Bug Fixes - Add LICENSE-3RD-PARTY.txt file ([#4304](#4304)) ([daa0fe6](daa0fe6)) - add Object.values polyfill for node <=6 ([#4274](#4274)) ([5eb867b](5eb867b)) - **aria-required-children:** avoid confusing aria-busy message in failures ([#4347](#4347)) ([591607d](591607d)), closes [#fail13](https://github.com/dequelabs/axe-core/issues/fail13) [#4340](#4340) - avoid reading element-specific node properties of non-element node types ([#4317](#4317)) ([b853b18](b853b18)), closes [#4316](#4316) [#4316](#4316) - **color-contrast:** handle text that is outside `overflow: hidden` ancestor ([#4357](#4357)) ([bdb7300](bdb7300)), closes [#4253](#4253) - **color-contrast:** support color blend modes hue, saturation, color, luminosity ([#4365](#4365)) ([7ae4761](7ae4761)) - **d.ts:** RawNodesResult issues ([#4229](#4229)) ([d660518](d660518)) - **d.ts:** RunOptions.reporter can be any string ([#4218](#4218)) ([e53f5c5](e53f5c5)) - **i18n:** update Italian translations ([#4377](#4377)) ([4d65d4b](4d65d4b)) - **listitem:** clarify roleNotValid message ([#4374](#4374)) ([0f8a9af](0f8a9af)) - **scrollable-region-focusable:** missing wcag213 tag ([#4201](#4201)) ([0080a72](0080a72)) - **target-size:** always pass 10x targets (avoid perf bottleneck) ([#4376](#4376)) ([be327c4](be327c4)) - **target-size:** do not crash for nodes with many overlapping widgets ([#4373](#4373)) ([1dbea83](1dbea83)), closes [#4359](#4359) [#4359](#4359) [#4360](#4360) - **utils/get-selector:** ignore 'xmlns' attribute when generating a selector ([#4303](#4303)) ([938b411](938b411)) This PR was opened by a robot 🤖 🎉
|
Hi @WilcoFiers and @straker - I've raised a bug here which I think could potentially relate to this change? Would you mind taking a look? Thanks! |
straker
added a commit
that referenced
this pull request
Apr 25, 2024
…ith prototype.js (#4429) This [puts back the v0.4.3 code](#4366) in `color.js` to handle colorjs.io not handling rad and turn values in hsl. I also decided to use core-js `Array.from` polyfill rather than our own internal one since I needed to bring it in outside of the `polyfill.js` file and could then add it as an import and reuse it in both places. Closes: #4428
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.
Decided to move the Color improvements from https://github.com/dequelabs/axe-core/pull/4365/files into their own PR. This also fixes the
clipbug mentioned in https://github.com/dequelabs/axe-core/pull/4365/files#r1517706612No QA needed