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

feat(page): support embed view for certain attachment types #5475

Merged
merged 13 commits into from
Nov 29, 2023

Conversation

lawvs
Copy link
Collaborator

@lawvs lawvs commented Nov 27, 2023

Fix #3641 #5033

This PR may need to undergo UI review.

There is a new property that has been added to the attachment model.

export type AttachmentBlockProps = {
  /**
   * Whether to show the attachment as an embed view.
   */
  embed: boolean | BackwardCompatibleUndefined;
}
Screen.Recording.2023-11-27.at.19.46.25.mov

Screenshot 2023-11-28 at 17 36 40

Known issues

  • The selection can not go through the iframe. The embed bookmark also has the same problem.

This issue can be resolved by applying a transparent layer over the iframe.

Screen.Recording.2023-11-28.at.02.19.09.mov
  • The PDF will reset to page 0 when the page reloads. I think it's reasonable, as it is difficult to control the expected page number unless we add a new options button.

Copy link

vercel bot commented Nov 27, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
blocksuite ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 28, 2023 4:25pm
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
blocksuite-docs ⬜️ Ignored (Inspect) Visit Preview Nov 28, 2023 4:25pm

Copy link
Member

@doodlewind doodlewind 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 update! We've discussed and confirmed this basically match the roadmap, but will be great if some UI setup could be optimized. For example, could you add #toolbar=0 to the embed PDF url, so that the PDF toolbar could be hidden?

@@ -212,6 +214,8 @@ export function isAttachmentLoading(modelId: string) {

/**
* Use it with caution! This function may take a long time!
*
* @deprecated
Copy link
Member

@doodlewind doodlewind Nov 28, 2023

Choose a reason for hiding this comment

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

Any recommended practice as alternative?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since the hasBlob is similar to getAttachmentBlob, I have migrated to getAttachmentBlob.

But neither of them is a best practice. the BlobManager needs to be refactored to meet the following requirements:

  • A new head API, also known as has, needs to be added to check a blob.
  • The get and set methods should return an Object that contains a stream to make it easier to check the progress of the upload or download.
  • The list API should have a default maximum limit and the ability to be adjusted by its options.

I think the cloudflare R2 workers API is a good reference.

Copy link
Member

@doodlewind doodlewind left a comment

Choose a reason for hiding this comment

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

Looks good to land, thanks!

@doodlewind doodlewind merged commit 40e2d56 into master Nov 29, 2023
18 checks passed
@doodlewind doodlewind deleted the feat/embed-attachment branch November 29, 2023 00:57
@doodlewind doodlewind mentioned this pull request Nov 29, 2023
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notable Major improvement worth emphasizing ui-style Related to UI styling
Projects
Archived in project
Archived in project
Development

Successfully merging this pull request may close these issues.

Support embedding audio and video
2 participants