-
Notifications
You must be signed in to change notification settings - Fork 407
✨(frontend) add pdf block to the editor #1293
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
Conversation
Hey team, This is a duplicate of PR-795, since I couldn’t reopen the original after losing the fork. I’d appreciate the maintainers’ input on a few points where the implementation could be improved:
Looking forward to your feedback and happy to adjust the implementation accordingly. |
Hey @dakshesh14 ! |
Hey @virgile-dev, happy to be back! No problem at all - I’ll wait for @AntoLC to return and review the PR. Thanks for keeping me posted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution, it is a great feature ! 🎉
Feel free to ask me if you need more details about the comments.
src/frontend/apps/impress/src/features/docs/doc-export/blocks-mapping/index.ts
Outdated
Show resolved
Hide resolved
src/frontend/apps/impress/src/features/docs/doc-editor/components/custom-blocks/PdfBlock.tsx
Outdated
Show resolved
Hide resolved
src/frontend/apps/impress/src/features/docs/doc-editor/components/custom-blocks/PdfBlock.tsx
Show resolved
Hide resolved
src/frontend/apps/impress/src/features/docs/doc-editor/components/custom-blocks/PdfBlock.tsx
Outdated
Show resolved
Hide resolved
const { wrapperRef, pdfWidth, handlePointerDown } = usePdfResizer( | ||
block.props.previewWidth ?? 100, | ||
handleResizeEnd, | ||
); | ||
|
||
return ( | ||
<div ref={contentRef} className="bn-file-block-content-wrapper"> | ||
{pdfUrl === '' ? ( | ||
<AddFileButton | ||
block={block} | ||
editor={editor as FileBlockEditor} | ||
buttonText="Add PDF" | ||
buttonIcon={<Icon iconName="upload" $size="18px" />} | ||
/> | ||
) : ( | ||
<FileBlockWrapper block={block} editor={editor as FileBlockEditor}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should leverage ResizableFileBlockWrapper
, it will simplify your component a lot.
ResizableFileBlockWrapper
includes the resizing logic, plus it includes the part when the url is not defined yet.
You can see how to use it here: https://github.com/TypeCellOS/BlockNote/blob/7a66f11fd8a21ce3c1ffcb87590ae44b7f877ed6/packages/react/src/blocks/ImageBlockContent/ImageBlockContent.tsx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be in another PR, but make ResizableFileBlockWrapper
sizable on height as well will be great !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used ResizableFileBlockWrapper
, but it made the whole PDF block a bit laggy to resize. I think the issue is with the <embed>
tag and not ResizableFileBlockWrapper
, since replacing <embed>
with a <div>
(for testing) removed the lag. What do you suggest I do here?
Adding an option to resize height sounds good, but if that works for you, I’d prefer to handle it in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decided to revert changes related to resizing to make the PR short. We can look into resizing in a separate PR along with the height resizing option.
src/frontend/apps/impress/src/features/docs/doc-editor/components/custom-blocks/PdfBlock.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to have some drag and drop issue with the embed tag, not sure why.
-.Docs.1.webm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure either.
src/frontend/apps/impress/src/features/docs/doc-export/blocks-mapping/pdfPdf.tsx
Outdated
Show resolved
Hide resolved
src/frontend/apps/impress/src/features/docs/doc-editor/components/custom-blocks/PdfBlock.tsx
Outdated
Show resolved
Hide resolved
Hi @AntoLC, thank you for your reviews! I will work on these. |
49aa5ae
to
b28786a
Compare
Hey @AntoLC, I’ve addressed all your review comments and also resolved the merge conflict. I decided to handle PDF resizing in a separate PR to keep this one focused and shorter. Could you please take another look when you get a chance? |
b28786a
to
123ca22
Compare
Added pdf block in the editor. Signed-off-by: dakshesh14 <[email protected]>
The way we were handling the antivirus upload loader was not optimal, it didn't work well with the pdf embed block. We created a dedicated upload loader block, it will replace the previous implementation, it is more Blocknote idiomatic and will work better with any type of upload files.
123ca22
to
cc4bed6
Compare
@dakshesh14 I took the freedom to finalize the PR to fit our needs (test e2e, manage loading upload with antivirus..). Thank you very much for this contribution, it is a very nice feature 🎉 !! Thanks 🙏 |
I totally understand - explaining those changes can often be harder than just fixing them directly, especially since you already have the full context of the system. I’m really glad you liked the feature! This PR ended up being bigger than I anticipated, so next time I’ll ensure that my changes are more focused and concise to make reviewing easier. I truly appreciate your efforts and patience throughout the process. Thanks again, @AntoLC and @virgile-dev 🙏 |
Purpose
Added an option for users to upload and preview PDF files in the editor. This PR addresses #348. It is a duplicate of PR-795 because I couldn't reopen the PR as I lost my fork.
pdf-embed.webm
Proposal
Added the option for users to upload and preview PDF files within the editor.
Description: