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

Refs #343 - Don't forget to render tail of text nodes #344

Merged
merged 1 commit into from
May 28, 2022

Conversation

claudep
Copy link
Collaborator

@claudep claudep commented May 19, 2022

No description provided.

@claudep claudep marked this pull request as draft May 19, 2022 19:44
@claudep
Copy link
Collaborator Author

claudep commented May 19, 2022

The patch may need some more work.

@lgtm-com
Copy link
Contributor

lgtm-com bot commented May 19, 2022

This pull request fixes 6 alerts when merging b96e17e into d153142 - view on LGTM.com

fixed alerts:

  • 6 for Variable defined multiple times

@lgtm-com
Copy link
Contributor

lgtm-com bot commented May 19, 2022

This pull request fixes 6 alerts when merging e2b9cc2 into d153142 - view on LGTM.com

fixed alerts:

  • 6 for Variable defined multiple times

@lgtm-com
Copy link
Contributor

lgtm-com bot commented May 19, 2022

This pull request fixes 6 alerts when merging 2ef3517 into d153142 - view on LGTM.com

fixed alerts:

  • 6 for Variable defined multiple times

@claudep claudep marked this pull request as ready for review May 20, 2022 16:52
@lgtm-com
Copy link
Contributor

lgtm-com bot commented May 20, 2022

This pull request fixes 6 alerts when merging da48109 into d153142 - view on LGTM.com

fixed alerts:

  • 6 for Variable defined multiple times

@claudep
Copy link
Collaborator Author

claudep commented May 20, 2022

@jspobst, if you want to test this PR, you are welcome!

@jspobst
Copy link

jspobst commented May 20, 2022

I just checked it out, and tested the example code I put in the issue. I can confirm that all of the text shows, but the formatting (ie, italics and bold) still doesn't work. The result:
image

If this PR was meant only to address some of the text missing before, I'd say it looks good (but doesn't fully close the corresponding issue). Admittedly, I might not understand all of what goes into 'testing this PR', so if that's more than simply generating one pdf, further testing by someone who understands best practices might be needed.

@claudep
Copy link
Collaborator Author

claudep commented May 20, 2022

I thought you might have other real SVGs that you might test.

The style issue is a completely different thing. svglib doesn't yet support the shorthand font, but only the more specific font-* properties.

@claudep claudep merged commit 4fdc3bc into deeplook:master May 28, 2022
@claudep claudep deleted the issue343a branch May 28, 2022 12:28
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