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

♻️(front) improve table pdf rendering #639

Merged
merged 1 commit into from
Feb 19, 2025
Merged

♻️(front) improve table pdf rendering #639

merged 1 commit into from
Feb 19, 2025

Conversation

NathanVss
Copy link
Contributor

Purpose

The previous way of rendering table was causing issues when tables could not fit on one page. I then came accross this discussion diegomura/react-pdf#2343. The author created a lib to improve the rendering of table, it's better, but still not perfect maybe.

Improving PDF is quite cumbersome, so I encourage you to test on your side to see if the rendering is correct. Otherwise we'll need to put more effort into this by going deep in the implementation, not sure we want to put that much of energy in it. Let's try this lib before :)

Proposal

The Doc

Capture d’écran 2025-02-14 à 15 32 46

Before

Capture d’écran 2025-02-14 à 15 32 22

After

Capture d’écran 2025-02-14 à 15 33 05

@NathanVss NathanVss force-pushed the fix/pdf-render branch 3 times, most recently from 65a3137 to 779e3c9 Compare February 14, 2025 15:22
@arnaud-robin
Copy link
Collaborator

Wow, this is cool !

Copy link
Collaborator

@AntoLC AntoLC left a comment

Choose a reason for hiding this comment

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

It's cool ^^
Let's see this yarn.lock and let's go 🚀

Copy link
Collaborator

Choose a reason for hiding this comment

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

I got lot of diff when I try the branch. I think the yarn.lock is not totally updated (rebase ?)

<TH key={index}>
{row.cells.map((cell, index) => {
return (
<TD key={index}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have a few warning about identical key, maybe it can be fix easily ?

image

@AntoLC
Copy link
Collaborator

AntoLC commented Feb 17, 2025

It could be nice to have it integrated by default in Blocknotes wdyt @YousefED ?

@YousefED
Copy link
Collaborator

It could be nice to have it integrated by default in Blocknotes wdyt @YousefED ?

Cool! Great to see this. I'd be happy to add this to react-pdf. @NathanVss are you able to do this? Otherwise we add it to our todos

I see two ways:
a) migrate to react-pdf-table in BlockNote
b) find the part in react-pdf-table that actually fixes the cross-page behavior (or the part in BlockNote that breaks it). I just spend some time to find this, but didn't immediately spot the cause

@AntoLC AntoLC self-requested a review February 19, 2025 12:15
Copy link
Collaborator

@AntoLC AntoLC left a comment

Choose a reason for hiding this comment

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

Nice improvement ^^

The previous way of rendering table was causing issues when tables
could not fit on one page. I then came accross this discussion
diegomura/react-pdf#2343. The author
created a lib to improve the rendering of table, it's better, but
still not perfect maybe.
@NathanVss NathanVss merged commit 009f5d6 into main Feb 19, 2025
18 of 19 checks passed
@NathanVss NathanVss deleted the fix/pdf-render branch February 19, 2025 12:58
@YousefED
Copy link
Collaborator

@NathanVss I was wrong in standup, react-pdf depends on pdfkit, not pdfjs

For pdfkit, this issue is in progress: foliojs/pdfkit#1577

Once that lands it should be possible to add it to react-pdf and then to blocknote / docs

This was referenced Mar 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants