Skip to content
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

Add support for decimals in hsla colors and some tests for strings->colors #9915

Merged
merged 7 commits into from
Jun 11, 2024

Conversation

williamforster
Copy link
Contributor

I have added support for numerals with decimal places when parsing hsla colors.

These can appear in svg files especially, they are otherwise rendered black.

I also added some jest tests to support this.

Thanks !

Copy link

codesandbox bot commented Jun 7, 2024

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders

Open Preview

@@ -99,7 +99,7 @@ export const reRGBa = () =>
* So the spec does not allow `hsl(30 , 45% 35, 49%)` but this will work anyways for us.
*/
export const reHSLa = () =>
/^hsla?\(\s*([+-]?\d{1,3})\s*[\s|,]\s*(\d{1,3}%)\s*[\s|,]\s*(\d{1,3}%)\s*(?:\s*[,/]\s*(\d*(?:\.\d+)?%?)\s*)?\)$/i;
/^hsla?\(\s*([+-]?[\d\.]+)\s*[\s|,]\s*([\d\.]+%)\s*[\s|,]\s*([\d\.]+%)\s*(?:\s*[,/]\s*(\d*(?:\.\d+)?%?)\s*)?\)$/i;
Copy link
Member

Choose a reason for hiding this comment

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

if the regex for RGBA for numbers with decimals are good enough, can you use the same notation in the regex?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I'll copy the regex from ReRGBa and re-test

@@ -254,12 +255,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 noNone = color.replaceAll('none', '0');
Copy link
Member

Choose a reason for hiding this comment

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

what is this none use case? is it specific of svg? in that case it should be solved in the svg parser ( in a different PR )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes svg allows you to use 'none' instead of '0'. I will move it to the parser

Copy link
Member

Choose a reason for hiding this comment

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

no leave it for another PR. don't keep adding stuff to the same pr. That is a different topic, 'none' is probably already handled somewhere

@asturur asturur merged commit 38ace43 into fabricjs:master Jun 11, 2024
20 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants