-
Notifications
You must be signed in to change notification settings - Fork 3
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 repeat to radialGradient #2
base: master
Are you sure you want to change the base?
add repeat to radialGradient #2
Conversation
Apparently the notifications were not set correctly and I was not receiving any of them. We'll be on it now. |
@@ -35,6 +35,21 @@ | |||
display:block; | |||
} | |||
|
|||
div.psudo-after::before { |
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.
psudo?
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.
Typo likely, pr is pretty old now. I'll spend some time and clean up bits.
|
||
processColorStops(backgroundImage.stops, rx * 2).forEach((colorStop) => | ||
radialGradient.addColorStop(colorStop.stop, asString(colorStop.color)) | ||
); | ||
|
||
this.path(path); | ||
this.ctx.fillStyle = radialGradient; | ||
this.path.bind({ctx, formatPath: this.formatPath.bind({ctx})})(normalisedPath); |
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.
The fact that you need to use bind seem suspicious. Can't we use arrow functions instead?
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.
I'll try take a look, this I believe was a workaround because background (html2canvas) didn't allow arguments again long time ago I'll take a look
@mikey0000 Can you illustrate the problem this is supposed to fix, maybe with some screenshots? Would be helpful. Thanks a lot! |
Should be a screenshot on the original pr? Tests should show how it works too? |
Anything that helps me understand a bit better the problem and the possible implications. |
So this is a link to the original PR niklasvh#2710 I'll re-pull this down and screenshot the test |
So looking over this, it resolves background-repeat: repeat-y; not being rendered. I'll take a look today and see if I can improve the code a bit. |
Summary
radial gradients do not repeat when they should
This PR fixes/implements the following bugs/features
Explain the motivation for making this change. What existing problem does the pull request solve?
adds the repeat functionality to the radial gradients.
Fixes niklasvh#2103
possibly fixes niklasvh#2620 niklasvh#2474
At this stage I know I need some feedback and direction as some of the changes aren't entirely tidy. Happy to discuss the direction you want to take this and planning some tests to cover it.