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 fonts with different font-weights #181

Merged
merged 5 commits into from Sep 14, 2021

Conversation

EugeneBalabai
Copy link
Contributor

Based on: parallax/jsPDF#3036

Now users can use any combination of font-weight and font-style

Copy link
Member

@HackbrettXXX HackbrettXXX left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I like this approach, because it remains compatible with jsPDF@<2.3.0. However, I think we still should implement it differently: we should pass font style and font weight as separate arguments to setFont. This way, we let jsPDF do the heavy lifting of combining them (which is more of an implementation detail, anyway). It would also be more futureproof, in case jsPDF improves the algorithm that matches a combination of font style and font weight with the available fonts.

In order to remain compatible with jsPDF@<2.3.0 we could switch between the old and new code paths depending on the jsPDF.version property.

@EugeneBalabai
Copy link
Contributor Author

Hey @HackbrettXXX

Thanks for review
yep, we can pass font-weight as separate arguments to setFont and I will do it (a bit later). But what about findFirstAvailableFontFamily? We still need to combine font style and font-weight with the same algorithm as jsPDF to get the correct fontFamily. Or do you have any ideas on how we can solve this?

@HackbrettXXX
Copy link
Member

Ah, good catch! Unfortunately, I don't have a better idea for findFirstAvailableFontFamily. I guess, If we're forced to combine font style/weight manually anyway, we can just take your initial implementation.

Could you have a look at the failing tests? Seems like some test cases produce different results now. The new results are probably correct, though. Please update the reference files in that case.

 - update complete-organization-chart-new to use a basic font
@HackbrettXXX
Copy link
Member

I've added some changes: the function for combining fonts was actually not correct in jsPDF. I've fixed it there (parallax/jsPDF#3217). The fix will be released with jsPDF@2.4.0. I've added a version switch to svg2pdf, so it will work with both jsPDF versions. The tests will fail until we update to jsPDF@2.4.0. We can do so, once it's released.

@EugeneBalabai
Copy link
Contributor Author

@HackbrettXXX great job! Do you know when it will release?

@HackbrettXXX
Copy link
Member

Hopefully soon ;)

@HackbrettXXX HackbrettXXX linked an issue Sep 14, 2021 that may be closed by this pull request
@HackbrettXXX HackbrettXXX merged commit 2de7de3 into yWorks:master Sep 14, 2021
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.

Support numerical values for font-weight attribute
2 participants