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

Added an example for using plotly with fpdf2 #714

Merged
merged 3 commits into from
Mar 8, 2023

Conversation

msalem99
Copy link

@msalem99 msalem99 commented Mar 7, 2023

Closes #653

Checklist:

  • The GitHub pipeline is OK (green), meaning that both pylint (static code analyzer) and black (code formatter) are happy with the changes of this PR.

  • [N/A] A unit test is covering the code added / modified by this PR

  • This PR is ready to be merged

  • [N/A] In case of a new feature, docstrings have been added, with also some documentation in the docs/ folder

  • A mention of the change is present in CHANGELOG.md

By submitting this pull request, I confirm that my contribution is made under the terms of the GNU LGPL 3.0 license.

@msalem99 msalem99 requested a review from Lucas-C as a code owner March 7, 2023 01:36
@codecov
Copy link

codecov bot commented Mar 7, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (caab96f) 93.75% compared to head (3343dc9) 93.75%.

❗ Current head 3343dc9 differs from pull request most recent head 26c0f41. Consider uploading reports for the commit 26c0f41 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #714   +/-   ##
=======================================
  Coverage   93.75%   93.75%           
=======================================
  Files          26       26           
  Lines        7008     7008           
  Branches     1258     1258           
=======================================
  Hits         6570     6570           
  Misses        260      260           
  Partials      178      178           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@Lucas-C Lucas-C left a comment

Choose a reason for hiding this comment

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

Welcome @msalem99, and thank you for your contribution! 😊

Good job overall, the example and documentation you provided are great! 👍

I tested your code, and while the 2nd script works well,
the 1st script hangs for several minutes on my computer, without ever completing.
I think it's stuck when calling the kaleido binary program.

How long does it take to execute on your machine?
I am using WSL2 (Ubuntu 20.04) under Windows, with Python 3.8.
What is your environment please?

docs/Maths.md Outdated Show resolved Hide resolved
docs/Maths.md Outdated Show resolved Hide resolved
@msalem99
Copy link
Author

msalem99 commented Mar 7, 2023

The second script seems to also require the installation of kaleido as plotly uses it when writing the svg figure. I tried running the second script without kaleido and it raised an error so I am not sure why kaleido would hang in the first script on your machine but run on the second script fine.

@Lucas-C
Copy link
Member

Lucas-C commented Mar 8, 2023

I think I may be facing this bug: plotly/Kaleido#149
And I haven't found a way to work around it for now...

I'm going to ask for some help 😊
@gmischler, @eroux, @andersonhc, @RubendeBruin : could one of you please test the script provided by our new contributor @msalem99 here?
I'm unable to do so, and just would like to make sure that at least one other person can run it before I merge this PR.

@Lucas-C
Copy link
Member

Lucas-C commented Mar 8, 2023

@ msalem99: also, could you add a line about this new documentation section in CHANGEGOG.md as part of this PR please?
You should easily find existing mentions in this file of documentation additions, that you can use as example 😊

@andersonhc
Copy link
Collaborator

I think I may be facing this bug: plotly/Kaleido#149 And I haven't found a way to work around it for now...

I'm going to ask for some help 😊 @gmischler, @eroux, @andersonhc, @RubendeBruin : could one of you please test the script provided by our new contributor @msalem99 here? I'm unable to do so, and just would like to make sure that at least one other person can run it before I merge this PR.

Both worked OK on my setup - python 3.10 running on Windows 10.
The images embedded in the PDF files are similar to the reference ones.

@msalem99
Copy link
Author

msalem99 commented Mar 8, 2023

@ msalem99: also, could you add a line about this new documentation section in CHANGEGOG.md as part of this PR please? You should easily find existing mentions in this file of documentation additions, that you can use as example 😊

Done, I also removed an import that was not used in the second example that I had missed before. Thank you for the feedback.

@Lucas-C
Copy link
Member

Lucas-C commented Mar 8, 2023

Thank you @andersonhc & @manager!
Merging this now 😊

@Lucas-C Lucas-C merged commit 3988367 into py-pdf:master Mar 8, 2023
@Lucas-C
Copy link
Member

Lucas-C commented Mar 8, 2023

@allcontributors please add @msalem99 for documentation

@allcontributors
Copy link

@Lucas-C

I've put up a pull request to add @msalem99! 🎉

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.

Doc: provide an usage example of combining fpdf2 with Plotly
3 participants