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

fix: font selection regression #2747

Merged
merged 5 commits into from
Jun 24, 2024

Conversation

andrew-spare
Copy link
Contributor

@andrew-spare andrew-spare commented May 8, 2024

This recent change #2640 appears to have caused a regression where the first font used in a line of text is utilized for the entire line. This includes usage of fontFamily, fontWeight, and fontStyle.

Changes

  • Fixed pickFontFromFontStack to select from fontStack before lastFont. This should now properly prioritize fonts
  • Added unit tests: packages/layout/tests/text/fontSubstitution.test.js
  • Updated example document to include this scenario

Related

Example

<Text style={{ fontSize: 10 }}>
  The following are multiple font families, weights, and styles all on the
  same line
</Text>
<Text style={{ fontFamily: 'Roboto' }}>
  Roboto Normal{' / '}
  <Text style={{ fontWeight: 'bold' }}>Roboto Bold</Text>
  {' / '}
  <Text style={{ fontStyle: 'italic' }}>Roboto Italic</Text>
  {' / '}
  <Text style={{ fontFamily: 'Courier' }}>Courier</Text>
</Text>

Before

example-before

After

example-after

Copy link

changeset-bot bot commented May 8, 2024

🦋 Changeset detected

Latest commit: 89e8f82

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
@react-pdf/examples Patch
@react-pdf/layout Patch
@react-pdf/renderer Patch
@react-pdf/e2e-node-cjs Patch
@react-pdf/e2e-node-esm Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@iSilkline
Copy link

@diegomura could you please merge this?

@TomasSalas
Copy link

@diegomura Plase merge this ! i need :(

@websnacks
Copy link

Yes I am using this library as well and right now the fonts are not working correctly. This fix is what I am waiting for! Please merge!

@TomasSalas
Copy link

@websnacks Dear, when can we see the change reflected in the library? I am waiting for this change to upload a code to improve a PDF

@TomasSalas
Copy link

When are the changes going to go up? I need it @diegomura @bdkopen

@Hatko
Copy link

Hatko commented May 22, 2024

I applied these changes, but still couldn't get the bold font back. Instead, to resolve it, I forced the resolution of "@react-pdf/layout" to point to 3.11.5. We are using bun, for it you need to add:

"overrides": {
    "@react-pdf/layout": "3.11.5"
},

to your package.json

@TomasSalas
Copy link

TomasSalas commented May 23, 2024

@Hatko @websnacks @diegomura
What you tell me doesn't work for me since I work with @react-pdf/renderer

I have not implemented @react-pdf/layout in my project

Captura de pantalla 2024-05-23 a la(s) 9 43 01 a  m Captura de pantalla 2024-05-23 a la(s) 9 43 19 a  m Captura de pantalla 2024-05-23 a la(s) 9 43 54 a  m Captura de pantalla 2024-05-23 a la(s) 9 46 15 a  m

This is where I have the problem

@lecstor
Copy link

lecstor commented May 23, 2024

@TomasSalas the fix needs to be applied to the @react-pdf/layout package which @react-pdf/renderer depends on.
I applied the fix using yarn patch @react-pdf/layout (as I use yarn for my package manager) and can confirm that it resolved the issue in our application.

https://yarnpkg.com/cli/patch

@TomasSalas
Copy link

@lecstor Could you help me with my project? since I work with NPM and I do not fully understand what you are telling me to be able to use what I need in the texts ??

@lecstor
Copy link

lecstor commented May 23, 2024

@TomasSalas it looks like you could use this then.. https://www.npmjs.com/package/patch-package

You'll need to open node_modules/@react-pdf/layout/lib/index.js in your project and apply the fix in this PR. (and do the same to node_modules/@react-pdf/layout/lib/index.cjs)

Then follow the instructions in patch-package to create the patch and commit it to your repo.

Screenshot 2024-05-24 at 7 58 39 AM

@TomasSalas
Copy link

@lecstor
Captura de pantalla 2024-05-23 a la(s) 6 09 44 p  m

I made the change in node modules and nothing changes

Captura de pantalla 2024-05-23 a la(s) 9 46 15 a  m

@lecstor
Copy link

lecstor commented May 23, 2024

@TomasSalas have a look at the PR. Did you remove lines 204 to 206?

@Elliot67
Copy link

@TomasSalas

What you tell me doesn't work for me since I work with @react-pdf/renderer

I believe @react-pdf/renderer uses @react-pdf/layout under the hood.
Try to pin a specific version of @react-pdf/layout as mentioned by @Hatko.

If you are using yarn, replace overrides with resolutions:

"resolutions": {
    "@react-pdf/layout": "3.11.5"
},

@TomasSalas
Copy link

@lecstor yes remove lines 204 to 206


Captura de pantalla 2024-05-23 a la(s) 6 09 44 p  m

@lecstor
Copy link

lecstor commented May 23, 2024

@TomasSalas not according to that screenshot in your last comment.
Please look at the PR changes.

These lines should not remain..

if (lastFont) {
  fontStackWithFallback.unshift(lastFont);
}
Screenshot 2024-05-24 at 8 21 44 AM

@TomasSalas
Copy link

@lecstor
It worked ! Thank you very much.. the last question, if this goes to production, will there be any problems? Or what should I do when building the app?

@lecstor
Copy link

lecstor commented May 24, 2024

@TomasSalas as long as you add the postinstall script as per the instructions the package should be patched whenever npm install is run, which should include during CI when deploying to prod.

@JustJoostNL
Copy link

I have applied the patch like above, but unfortunately it's still not working (font weight).

@TomasSalas
Copy link

Is it known when the changes will be uploaded? @diegomura @bdkopen

@bdkopen
Copy link
Contributor

bdkopen commented Jun 13, 2024

Is it known when the changes will be uploaded? @diegomura @bdkopen

I am not a maintainer, so I have no control over when this will be merged.

@TomasSalas
Copy link

@bdkopen And do you know who the maintainer is? so I can request his help?

@bdkopen
Copy link
Contributor

bdkopen commented Jun 13, 2024

@bdkopen And do you know who the maintainer is? so I can request his help?

@diegomura is the primary maintainer.

gius pushed a commit to eManPrague/react-pdf that referenced this pull request Jun 14, 2024
update example

add tests

fix mistakes
@C4T4
Copy link

C4T4 commented Jun 14, 2024

@diegomura merge needed

@TomasSalas
Copy link

@diegomura please 🙏🏻

@iSilkline
Copy link

@diegomura It's been 2+ months in the last merge. Are you still able to maintain this repo?

Copy link
Owner

@diegomura diegomura left a comment

Choose a reason for hiding this comment

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

utACK

@diegomura diegomura merged commit 5af35ec into diegomura:master Jun 24, 2024
@TomasSalas
Copy link

TomasSalas commented Jun 26, 2024

I just updated the library and it doesn't work @diegomura

@iSilkline
Copy link

@TomasSalas it's because it's not released yet. last release is april 25

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