Skip to content

fix: Colors can be stringified with expontential alphas, but cannot b…#66

Merged
Qix- merged 8 commits intoQix-:masterfrom
babycannotsay:master
Dec 3, 2021
Merged

fix: Colors can be stringified with expontential alphas, but cannot b…#66
Qix- merged 8 commits intoQix-:masterfrom
babycannotsay:master

Conversation

@babycannotsay
Copy link
Copy Markdown
Contributor

Copy link
Copy Markdown
Owner

@Qix- Qix- left a comment

Choose a reason for hiding this comment

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

One nit, and can you add a number of tests under the "invalid" section(s) that test a few invalid inputs? Such as 1e, e, 0e-, etc.

Comment thread index.js Outdated
Comment on lines +139 to +142
/**
* scientific-notation regexp
* [+-]?(?=\.\d|\d)(?:0|[1-9]\d*)?(?:\.\d*)?(?:[eE][+-]?\d+)?
*/
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This comment is unnecessary.

Suggested change
/**
* scientific-notation regexp
* [+-]?(?=\.\d|\d)(?:0|[1-9]\d*)?(?:\.\d*)?(?:[eE][+-]?\d+)?
*/

@babycannotsay
Copy link
Copy Markdown
Contributor Author

There's a compatibility issue.
Before hsla(0, 0%, 0%, +.) is same as hsla(0, 0%, 0%, 1)
but now hsla(0, 0%, 0%, +.) will throw error.

@Qix-
Copy link
Copy Markdown
Owner

Qix- commented Dec 3, 2021

That's fine, +. is not valid input. Anyone relying on that is masking a bug.

Copy link
Copy Markdown
Owner

@Qix- Qix- left a comment

Choose a reason for hiding this comment

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

Can you add tests for decimal notations as well? E.g. 127.88e4, 0.2e3, .1e-4, etc. Right now only 1e.. is being tested.

@babycannotsay
Copy link
Copy Markdown
Contributor Author

babycannotsay commented Dec 3, 2021

I wanted to save the scientific notation regular expression in a variable (and then write a test case), and pass it to new Regexp to concatenate it with the previous regular expression, but it was a lot of escaping / and made the regular expression even harder to understand.

Copy link
Copy Markdown
Owner

@Qix- Qix- left a comment

Choose a reason for hiding this comment

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

Sorry to keep nitting but also add them for hsl too please since they're two separate regexes.

@Qix- Qix- merged commit 94a429e into Qix-:master Dec 3, 2021
@Qix-
Copy link
Copy Markdown
Owner

Qix- commented Dec 3, 2021

Sorry, my food arrived right as I published and I got sidetracked. Quesadillas take priority in my life.

Thanks for the PR, published as 1.9.0!

Copy link
Copy Markdown

@dink42 dink42 left a comment

Choose a reason for hiding this comment

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

  • Sorry if i destoy your fixes! Tell me if im totally lost, otherwise thanks!

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.

4 participants