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

add repeat to radialGradient #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mikey0000
Copy link

@mikey0000 mikey0000 commented Jan 25, 2024

Summary

radial gradients do not repeat when they should

This PR fixes/implements the following bugs/features

  • radial gradients can repeat

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.

@Sharcoux
Copy link

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 {

Choose a reason for hiding this comment

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

psudo?

Copy link
Author

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);

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?

Copy link
Author

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

@Sharcoux
Copy link

@mikey0000 Can you illustrate the problem this is supposed to fix, maybe with some screenshots? Would be helpful. Thanks a lot!

@mikey0000
Copy link
Author

@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?

@Sharcoux
Copy link

Anything that helps me understand a bit better the problem and the possible implications.

@mikey0000
Copy link
Author

mikey0000 commented Mar 19, 2024

So this is a link to the original PR niklasvh#2710

I'll re-pull this down and screenshot the test

@mikey0000
Copy link
Author

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.

@mikey0000
Copy link
Author

Screenshot_20240319_180808
as horrible as it is, the vertical dots is what this pr resolves that html2canvas ignores

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.

CSS radial-gradiant does not have correct direction background-repeat not working
2 participants