-
-
Notifications
You must be signed in to change notification settings - Fork 56
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(gradients): support for 'transparent' as a color stop #65
Conversation
AFAIK I am proposing to replace calls to |
Example output: > temp1
CanvasRenderingContext2D {canvas: canvas, globalAlpha: 1, globalCompositeOperation: "source-over", filter: "none", imageSmoothingEnabled: true, …}
> temp1.fillStyle = "transparent"
"transparent"
> temp1.fillStyle
"rgba(0, 0, 0, 0)" |
Another way to tackle this problem is to submit a pull request to the If perhaps they don't want to support this request, another option would be to fork I will accept any of these solutions to get things working. |
A module called validate-color can be used instead of parse-color, it validates colors by keyword, rgb[a] ,hex, and hsl[a] correctly. |
Wow this is great. Please give me a day to look over this and do a review. |
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.
Please make sure that "validate-color" actually returns the HEX values for the fillStyle and strokeStyle property output.
Further than validating them, to alter the format of provided colors to hex would require functionality more than |
Is it possible we could submit a pull request to the module we currently use? I bet the maintainer would appreciate it. |
If not, https://www.npmjs.com/package/color-rgba looks like it might be the module we need to use. |
The original module hasn't been touched in 6 years, which doesn't inspire hope in me that an issue would be answered
|
Yes. This looks like an opportunity to fork |
|
|
The defintely seems like the best option, I'll try it out |
I've read over the HTML canvas spec and updated this to follow the color-serialization rules faithfully. Internally, the |
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.
A lot of the above comments reference changes to the test suite which should not be changed to validate that the functions and properties are spec compliant.
Thank you for your hard work, and it looks like this pull request is almost done.
Please make sure that your coding style matches single quotes for strings.
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.
Another round of changes. Sorry if I wasn't clear about my suggestions. I would prefer to store the resulting color value up front, otherwise we will need to call into MooColor
more than once per proptery set
and get
.
I believe that some of this is my fault for not making my suggestions more clear.
In a future PR, I will add prettier support and a CONTRIBUTING.md
document to help others in the future.
Just to ask, I'd like to include a test suite for the new function |
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.
LGTM
@hustcc You can review this if you would like.
One of the things I noted about this pull request is that we should have a CONTRIBUTING.md
file and use prettier
for code formatting. I will submit a pull request later this week for these changes.
I don't think this utility function is necessary, and we only need to snapshot and test the endpoint output. If perhaps you would like to test the function more, we can add more colors to the test suite, but it does look like everything is working as intended. Another very good idea might just be to comment that function and state its purpose. |
One last commit suggestion:
|
No other modules I found could be used plug'n'play here. It seems that the only value that parse-color doesn't handle correctly is
transparent
, so it can be handled especiallySome test cases that verify different color formats get handled correctly wouldn't hurt either.
resolves #64