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 font-weight and font-style support #535

Merged
merged 7 commits into from
Mar 23, 2019
Merged

Add font-weight and font-style support #535

merged 7 commits into from
Mar 23, 2019

Conversation

diegomura
Copy link
Owner

@diegomura diegomura commented Mar 16, 2019

What this PR do

This adds the ability to setup font-styles and font-weights when registering fonts:

Font.register(`http://localhost:3000/Roboto-Regular.ttf`, {
  family: 'Roboto'
})

Font.register(`http://localhost:3000/Roboto-Bold.ttf`, {
  family: 'Roboto',
  fontWeight: 'bold'
})

Font.register(`http://localhost:3000/Roboto-Italic.ttf`, {
  family: 'Roboto',
  fontWeight: 'normal',
  fontStyle: 'italic'
})

Font.register(`http://localhost:3000/Roboto-BoldItalic.ttf`, {
  family: 'Roboto',
  fontWeight: 'bold',
  fontStyle: 'italic'
})

When rendering, react-pdf will choose the appropriate font in the same way the web does.

"Bulk" font loading

Since we now support referring to different font files by using the same fontFamily value, IMO would be cool to be able to load one fontFamily in one register call:

Font.register({
  family: 'Roboto',
  fonts: [
    {  
      src: 'http://localhost:3000/Roboto-Regular.ttf',
      fontWeight: 'normal', // Default, does not have to be specified
      fontStyle: 'normal',  // Default, does not have to be specified
    },
    {  
      src: 'http://localhost:3000/Roboto-Bold.ttf',
      fontWeight: 'bold' // Also accepts numeric values, ex. 700
    },
    {  
      src: 'http://localhost:3000/Roboto-Italic.ttf',
      fontStyle: 'italic'
    },
    {  
      src: 'http://localhost:3000/Roboto-BoldItalic.ttf',
      fontStyle: 'italic',
      fontWeight: 'bold',
    },
  ]
})

Other options such as method, headers and body that we currently support should still work for each font.

Change register API

We currently pass the font src in the first argument to Font.register, and all other options into the second. To achieve the latter point (and if we want to support CSS @font-face attribute in the future) it would be cool to have the API like this:

Font.register({
  src: '...',
  fontStyle: 'normal',
  fontWeight: 'bold'
})

We should still support the current API, by showing a deprecated warning

TODO

  • Add font-weight and font-style support
  • Bulk loading
  • Change register API
  • Update docs
  • Update REPL example
  • Update typings
  • Check typings correctness
  • Fix old tests
  • Add new tests

@DuncanMacWeb inputs?

Fixes #402

@diegomura diegomura changed the title Add font-weight support Add font-weight and font-style support Mar 16, 2019
@DuncanMacWeb
Copy link
Contributor

@diegomura Thanks so much for doing this! I’ll look through it tomorrow and let you know my thoughts. (Sorry, I wanted to focus on pull 538 these last couple of days and get it done.)

@diegomura
Copy link
Owner Author

Thank you! I'll review also your PR tomorrow 😄
I'm currently working on those last points so we can maybe merge this soon. I'll specially appreciate if you can review the typings, since it's a field that I don't have that much experience haha

@DuncanMacWeb
Copy link
Contributor

In summary, LGTM! I like how you’ve implemented this. I’ve used TypeScript before but I’m not currently using it, so I would like to check your syntax with a TS linter. But without checking, your typings look good.

I think there may be an opportunity to optimise data usage by only downloading registered fonts once they are actually used, but this can go in separately somehow.

@diegomura
Copy link
Owner Author

Thank you @DuncanMacWeb !!
Your feedback is REALLY appreciated.

I think there may be an opportunity to optimise data usage by only downloading registered fonts once they are actually used, but this can go in separately somehow.

This actually happens now. We don't download the font when it get's registered, but when it's loaded. This method get's called before rendering the doc only for the font descriptors present on the document

What would be cool in the future is creating a font subset only with the used glyphs, but that's something that should be implemented in textkit

@diegomura
Copy link
Owner Author

Btw. is there an easy way of running a TS linter?

@diegomura diegomura merged commit 8a111da into master Mar 23, 2019
@diegomura diegomura deleted the font-weight branch March 23, 2019 00:42
@DuncanMacWeb
Copy link
Contributor

@diegomura heads up, there appears to be a bug in this PR that causes fonts to be re-downloaded multiple times. Have you also observed this?

@diegomura
Copy link
Owner Author

diegomura commented Mar 27, 2019

Oops. No! I'll take a look at it asap. Good thing it's not published in npm yet

@diegomura
Copy link
Owner Author

@DuncanMacWeb fixed in #547
Sorry and thanks for reporting this!!
Have you been trying the new font loading support? Besides this, how it has been working?
Merging this today and maybe its time to publish these changes

@DuncanMacWeb
Copy link
Contributor

We haven’t tried it yet — will report back!

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