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

Implement fill-opacity on Path objects with gradients. #5812

Merged
merged 5 commits into from
Jul 21, 2019
Merged

Implement fill-opacity on Path objects with gradients. #5812

merged 5 commits into from
Jul 21, 2019

Conversation

ajvincent
Copy link
Contributor

This fixes the smaller issue in #5810 . That said, I'm not sure I put the test in the right place. It took me longer than I would've liked just to craft a test that works.

Please review with a bit of caution, and if the test is in the wrong place, please provide a better test for the right file. (I'm still quite an amateur at the fabric.js codebase.)

src/elements_parser.js Outdated Show resolved Hide resolved
@asturur
Copy link
Member

asturur commented Jul 20, 2019

As soon this is corrected and merged i can look in the geometry problem.

@ajvincent
Copy link
Contributor Author

I am uncomfortable with this latest patch, because I have changed the API to fabric.Gradient.fromElement: specifically, the second argument has become the third, and the opacity attribute has become the second attribute. Feel free to reject (again) if you would rather the new opacity attribute be appended to the end of the arguments to preserve API.

@asturur
Copy link
Member

asturur commented Jul 20, 2019

I agree that would be better to preserve the argument order, even if fromElement is supported just for internal use for the whole SVG parser.
But I really cannot predict if someone is using it stand alone, and if they managed succesfully would be nice to do not break the API for them.

@asturur
Copy link
Member

asturur commented Jul 21, 2019

i will add you svg example as a visual test ( that will probably fail for now )

@asturur asturur merged commit f0093e0 into fabricjs:master Jul 21, 2019
@ajvincent ajvincent deleted the path-gradient-opacity branch July 21, 2019 16:19
@asturur asturur mentioned this pull request Aug 19, 2019
thiagocunha pushed a commit to thiagocunha/fabric.js that referenced this pull request Nov 18, 2019
* Implement fill-opacity on Path objects with gradients.

* Reorder fabric.Gradient.fromElement arguments.
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.

2 participants