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

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

Merged
merged 8 commits into from
Dec 3, 2021

Conversation

babycannotsay
Copy link
Contributor

Copy link
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.

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

Qix- commented Dec 3, 2021

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

Copy link
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
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
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
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

@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