-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
feat(Color) Regex and color code cleanup and new color format support #8916
Conversation
Build Stats
|
src/color/Color.ts
Outdated
@@ -279,7 +279,7 @@ export class Color { | |||
* @return {TRGBAColorSource | undefined} source | |||
*/ | |||
static sourceFromRgb(color: string): TRGBAColorSource | undefined { | |||
const match = color.match(reRGBa); | |||
const match = color.match(new RegExp(reRGBa, 'i')); |
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.
the truth is that the best code is the literal regex inside the function here.
The constants outside the file are an organization thing that we pay with having those in the scope of the library, always initialized.
changed my mind. |
s = parseFloat(match[2]) / (/%$/.test(match[2]) ? 100 : 1), | ||
l = parseFloat(match[3]) / (/%$/.test(match[3]) ? 100 : 1); | ||
s = parseFloat(match[2]) / 100, | ||
l = parseFloat(match[3]) / 100; |
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.
s and l are always percentages only.
Non percentages aren't ammitted.
At this point this needs some extra test for the new format covered |
…nto regex-color-cleanup
src/color/Color.ts
Outdated
* @return {TRGBColorSource} Hsl color | ||
*/ | ||
_rgbToHsl(r: number, g: number, b: number): TRGBColorSource { | ||
_rgbToHsl(r: number, g: number, b: number, a: number): TRGBAColorSource { |
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.
this should probably be an utility like hsltorgb
1ad6d00
to
77d2dbb
Compare
ok i can stop here with this. |
@Lazauya since you had previously worked with regexes, have a look for suggestions |
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.
This PR is a GREAT cleanup!
Some recommended changes
const [r, g, b] = this.getSource(); | ||
return `${hexify(r)}${hexify(g)}${hexify(b)}`; | ||
const fullHex = this.toHexa(); | ||
return fullHex.slice(0, 6); |
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.
why isn't it prefixed with #
?
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.
was like this before, trying to not change behaviour since it doesn't seems important.
I don't know the reasons, we should dig into git history
this.setSource([average, average, average, currentAlpha]); | ||
const [r, g, b, a] = this.getSource(), | ||
average = Math.round(r * 0.3 + g * 0.59 + b * 0.11); | ||
this.setSource([average, average, average, a]); | ||
return this; |
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.
Not very related, but since I am reviewing
Do we want to return a new instance for the toXXX
methods?
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.
this would be a possible change, and we can do that outside the cleanup/refactor scope.
Something that also should be done maybe that is part of cleanup is consolidathing this average code in a function util
@@ -310,14 +251,14 @@ export class Color { | |||
* @see http://http://www.w3.org/TR/css3-color/#hsl-color | |||
*/ | |||
static sourceFromHsl(color: string): TRGBAColorSource | undefined { | |||
const match = color.match(reHSLa); | |||
const match = color.match(reHSLa()); | |||
if (!match) { | |||
return; | |||
} | |||
|
|||
const h = (((parseFloat(match[1]) % 360) + 360) % 360) / 360, |
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.
const h = (((parseFloat(match[1]) % 360) + 360) % 360) / 360, | |
const h = ((parseFloat(match[1]) + 360) % 360) / 360, |
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.
i wonder... this seems correct and i noticed was off.
I m trying to think of some non obvious value that could break.
But i prefer to discover in this case and in case fix later because this seems wrong
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.
Ahhh I see.
What if the value is < -360. Now it fails, before it was safe.
Please revert this.
I didn't think of that.
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.
Yeah that is fine, we discovered.
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.
This is great!
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.
in my head there was the fun experiment that 150 colors can be represented by 2 digits in base64 encoding. And so that should be possible to reduce this map to have custom hashed keys to which color names resolve univocally. But i m not sure is easy or hard, but for sure would be fun.
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.
So desu, ne?!
export const reRGBa = | ||
/^rgba?\(\s*(\d{1,3}(?:\.\d+)?%?)\s*,\s*(\d{1,3}(?:\.\d+)?%?)\s*,\s*(\d{1,3}(?:\.\d+)?%?)\s*(?:\s*,\s*((?:\d*\.?\d+)?)\s*)?\)$/i; | ||
export const reRGBa = () => | ||
/^rgba?\(\s*(\d{0,3}(?:\.\d+)?%?)\s*[\s|,]\s*(\d{0,3}(?:\.\d+)?%?)\s*[\s|,]\s*(\d{0,3}(?:\.\d+)?%?)\s*(?:\s*[,/]\s*(\d{0,3}(?:\.\d+)?%?)\s*)?\)$/i; |
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.
does it over complicate reusing some string e.g. \(\s*(\d{0,3}(?:\.\d+)?%?)\s*[\s|,]\s*
in a string template?
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.
the issue with that is that i loose the ability to use the literal regex and i have to go with newRegexp.
I started with that if you look at the first commits. I got 400 extra bytes of code and i was pissed off.
I should try again now that i made the regex slightly different.
src/color/util.ts
Outdated
return hexValue.length === 1 ? `0${hexValue}` : hexValue; | ||
} | ||
export const hexify = (value: number) => | ||
Math.min(value, 255).toString(16).toUpperCase().padStart(2, '0'); |
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.
padStart, who knew. 👍
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.
This is exactly where we should verify rounding is not an issue.
var oColor = fabric.Color.fromRgba(stringToParse); | ||
assert.ok(oColor); | ||
assert.ok(oColor instanceof fabric.Color); | ||
assert.deepEqual(oColor.getSource(), expectedSource); | ||
var oColorUppercase = fabric.Color.fromRgb(stringToParse.toUpperCase()); | ||
assert.ok(oColorUppercase); | ||
assert.ok(oColorUppercase instanceof fabric.Color); | ||
assert.deepEqual(oColorUppercase.getSource(), expectedSource); |
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.
seems this function can be reused in the hsl loop as well
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.
could but i didn't becuase fromRgb
vs fromHsl
and didn't want to overcomplicate it. Still better than all different tests.
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.
thought of
function runColorTest({ name, stringToParse, expectedSource }, from, fromAlpha) {
var oColor = fromAlpha(stringToParse);
assert.ok(oColor);
assert.ok(oColor instanceof fabric.Color);
assert.deepEqual(oColor.getSource(), expectedSource);
var oColorUppercase = from(stringToParse.toUpperCase()); // fromAlpha??
assert.ok(oColorUppercase);
assert.ok(oColorUppercase instanceof fabric.Color);
assert.deepEqual(oColorUppercase.getSource(), expectedSource);
}
Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com>
Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com>
Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com>
Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com>
Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com>
Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com>
action items:
|
575a3c5
to
4b3693f
Compare
src/color/util.ts
Outdated
* Calculate the grey average value for rgb and pass through alpha | ||
*/ | ||
export const greyAverage = ( | ||
r: number, | ||
g: number, | ||
b: number, | ||
a = 1 | ||
): [average: number, alpha: number] => [ | ||
Math.round(r * 0.3 + g * 0.59 + b * 0.11), | ||
a, |
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.
seems alpha doesn't belong here
but I understand you want it the usage to be simple to use (less lines of code)
I suggest then returning a source value.
[greyAvg, greyAvg, greyAvg, alpha]
It is a bit weird like this, but not a big deal.
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.
yes maybe we can return the source format, is an internal util, it can do whatever it needs to do to make the code efficient
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 can make it from source to source.
Easier.
ok toHex and toHexa had a decimal bug, but now is forced as rounded |
I think i addressed all comments |
Motivation
Improve the regexes and the documentation around them.
So the part with strings and composition was definitely the most readable.
But took 400 bytes of extra code where usually we fit a small feature or a fix or 2.
Now we don't read regexes as a flow, we want to be able to know what they do and i wrote extensive docs for them
Changes
Following
https://developer.mozilla.org/en-US/docs/Web/CSS/color_value/rgb
and
https://developer.mozilla.org/en-US/docs/Web/CSS/color_value/hsl
80%
/
for the alpha channelGist
In Action