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

Opacity - semi transparent objects #487

Merged
merged 10 commits into from Jun 17, 2020
Merged

Conversation

soadzoor
Copy link
Contributor

Hey @Hopding ,

So I thought I'd submit a PR for #210 , as you suggested yesterday.

Some notes:

  • I've tested it, it works for my case, but there might be some cases I've missed.
  • Maybe I shouldn't push a new graphic state if both opacity and borderOpacity is 1 (or left untouched, as the default value is 1 for both of them)? I'm not sure about best practices when it comes to PDF.
  • Since it's only for svgPaths so far, I'd still need to add these things to drawText (as in the original issue), and a lot of other stuff (drawLine, drawCircle, drawSquare, drawImage, etc). In that case, I should try to avoid most of the code duplication related to this feature with private methods for example. Please let me know if you have any preferences, thoughts, warnings about this!

Have a great week!

@soadzoor soadzoor closed this Jun 15, 2020
@soadzoor soadzoor reopened this Jun 15, 2020
@soadzoor
Copy link
Contributor Author

... I wanted to create a draft pull request, but I misclicked. Now I can't see an option to change this to a draft PR.. Any ideas? :(

@soadzoor
Copy link
Contributor Author

@Hopding I've updated this PR, so now it contains opacity (and borderOpacity) for all the draw options (text, image, page, rectangle, line, square, ellipse, circle, svgpath)

@Hopding
Copy link
Owner

Hopding commented Jun 16, 2020

@soadzoor Thanks for working on this! I'll take a look. No worries about the draft PR. I don't think the status can be changed once it's opened, but that's no big deal.

@Hopding
Copy link
Owner

Hopding commented Jun 16, 2020

@soadzoor Can you please update a few of the integration tests to use this feature?

@soadzoor
Copy link
Contributor Author

@Hopding Sure, if you tell me how to do that! :)

@Hopding
Copy link
Owner

Hopding commented Jun 16, 2020

@soadzoor
Copy link
Contributor Author

@Hopding I haven't checked that one yet! It's past midnight here, so I'll do it tomorrow if that's okay :)

@Hopding
Copy link
Owner

Hopding commented Jun 16, 2020

Sounds good! No rush

@soadzoor
Copy link
Contributor Author

@Hopding I've updated test 2, 4, and 12, so now they contain some kind of opacity. I manually copy-pasted the same modifications from html to node/ts, deno/ts, and rn/js, and I'm not sure if that's the proper way of doing it. I didn't feel very professional while I was doing it, but for tests, it might be okay, I don't know. Please let me know what you think!

@Hopding
Copy link
Owner

Hopding commented Jun 17, 2020

@soadzoor The tests look good! Thanks!

And yes, there's a lot of copy/pasting between the tests for Node, Deno, React Native, and the Browser. It does feel a bit clunky, but it is by far the simplest approach and isn't a big deal since they're just tests. In a previous version of pdf-lib I tried to avoid the copy/pasting and reuse the tests. It turned into a complicated mess because each environment needs things to be a little bit different.

@soadzoor
Copy link
Contributor Author

@Hopding I see your point, it's completely fine for tests, I think. :)
I don't want to sound pushy, and I know you still need to review the code, but when do you think this PR will be merged? Just a rough estimation is okay!

Let me know if you'd need any modifications to this!

@Hopding
Copy link
Owner

Hopding commented Jun 17, 2020

I'll probably merge it this evening. But I might make a few tweaks and also need to run through all the tests before cutting a release. So I'd expect to cut a release with this feature sometime this weekend.

@soadzoor
Copy link
Contributor Author

@Hopding Sounds good! Thank you!

@Hopding Hopding merged commit 238f5f4 into Hopding:master Jun 17, 2020
Hopding pushed a commit that referenced this pull request Aug 30, 2021
* Added opacity and borderOpacity to drawSvgPath options

* Added opacity and borderOpacity to PDFPageDrawSVGOptions

* Added opacity and borderOpacity to PDFPage options

* Added ability to set opacity and borderOpacity for svgpath

* Fixes

* Some cleanup

* Wrapped functionality to private methods + added opacity feature for drawImage

* Added opacity to rectangle, square, ellipse, circle, text, line

* Added opacity for drawpage

* Updated a few integration tests with the new opacity features

Co-authored-by: Peter Varga <peter.varga@xyicon.com>
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.

None yet

2 participants