-
-
Notifications
You must be signed in to change notification settings - Fork 83
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
Port parse tests #427
Port parse tests #427
Conversation
✅ Deploy Preview for colorjs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Hey, thanks so much! Yes, Copilot was a LIFESAVER when porting these.
See a few comments, but none of them are blockers.
name: "hsl()", | ||
tests: [ | ||
{ | ||
name: "hsl(), commas", |
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 the output not show the parent name before the child name or in a way that implies containment?
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.
It does, the function portion of the name is redundant
expect: '{"spaceId":"hsl","coords":[90,0,0],"alpha":0.5}' | ||
}, | ||
{ | ||
name: "hsla(), rad for hue, spaces and slash", |
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 (and some of the ones after) mention hsla()
but use hsl()
, is that what the original tests did?
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.
That's what the original tests did.
I'm going to leave the redundant/incorrect hsl functions in the test names for now. I don't think hsl is spec compliant so I'll open a new issue for that (#428). The tests can be corrected as part of dealing with that issue. |
Closes #393
Shoutout to Github Copilot!