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: Default image blocks #331

Merged
merged 62 commits into from
Oct 3, 2023
Merged

feat: Default image blocks #331

merged 62 commits into from
Oct 3, 2023

Conversation

matthewlipski
Copy link
Collaborator

@matthewlipski matthewlipski commented Aug 27, 2023

This PR adds an image block to the default blocks. I have managed to implement most of the functionality available in other editors, namely:

  • Captions
  • Selection
  • Formatting Toolbar
  • Resize
  • Upload

The last thing to sort out is a default backend for image upload. I implemented one using imgbb.com but there are definitely better solutions.

Also there is one remaining bug that I haven't been able to fix - you cant click on items in the color picker dropdown when there is an image or captionedImage block anywhere in the editor. @YousefED it would be great if you could have a quick look at it, I don't think it should be super deep but also I couldn't figure it out.

@vercel
Copy link

vercel bot commented Aug 27, 2023

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

Name Status Preview Updated (UTC)
blocknote-website ✅ Ready (Inspect) Visit Preview Oct 2, 2023 3:03pm

@matthewlipski matthewlipski mentioned this pull request Aug 27, 2023
@YousefED
Copy link
Collaborator

YousefED commented Aug 28, 2023

Captions: Inline content is used for the image caption. This is great for formatting & rich text editing, but means we need 2 block types, one with caption and one without, due to the TipTap schema being fixed at runtime. This means there's also no clear visual distinction for a caption vs a paragraph block.

Seems like there are two options here, right?
A) always use an image with caption, but with a prop hasCaption: boolean that will enable / disable rendering of the caption. I'm not 100% sure if that's a possibility (i.e.: will things break if we don't render the InlineContent when the prop is false.
B) Indeed, use two block types, and when a user wants to add a caption to the image, we need to replace the existing block for ImageWithCaption and vice-versa. Seems not ideal

For the next steps, I think the major one is deciding whether the caption should stay as inline content or if it should be changed to a plain text prop. The advantages would be no longer needing 2 block types and having more consistent selection/formatting toolbar behaviour, but we would ofc lose rich text editing.

I think if I need to make a trade-off for a first version, I'd prefer a simple caption (or even no caption) and more polished other functionalities, than "compromise" on other functionalities but have a rich-text caption.

We also need to figure out how to handle image uploads properly as while Uppy itself works nicely, it still needs a backend.

A "sample" upload function implementation could use browser Blob / IndexedDB for storage, or simple convert the image to a base64 data uri and store that as "URI" in the prosemirror state. For the website demo we'll need to figure out something else indeed, as images will need to be shared among users - I can look into this later.

Copy link
Collaborator

@YousefED YousefED left a comment

Choose a reason for hiding this comment

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

This looks like a great start. I haven't done an in-depth code-review or looked at the color bug), but have added some small observations in the comments.

My thoughts at this moments are that these would be the most important things to focus on next:

  • Selectability / copy-paste:
    -- as a user, I would like to be able to select the image by clicking on it and see that it's selected
    -- as a user, I would like to be able to drag the image to another place in the document
    -- as a user, I want to be be able to copy/paste images within the editor, and to/from other places on the web
  • Configuring-flow. As you mentioned, currently we render the Uppy dialog inside the block. I think this mixes the "document state" with "editing state" (for example, in a multiplayer session it would mean all users see the uppy dialog). I think this is questionable, but on the other hand I see Notion (and Slite) does something similar where they show "Add an image" after inserting an image block. I think we have two options:
    -- A) don't have an "editing state"; only insert the image node once we have a URL (a bit similar to what we do with links)
    -- B) take a notion style approach where we only render a button instead of the entire Uppy UI, but only show the Uppy UI once this button is clicked.
    -- C) keep as is, if option A / B are too much work
  • Extract Uppy to separate package. Now we start making more complex blocks, I think we need to separate some functionality to optional packages, so that they don't need to pull in all dependencies such as the Uppy integration if they're not required (this will also become relevant when we create a code-block or table/grid block). Users should be able to customize their own "image selection" UI, or drop-in our Uppy component
  • React vs vanilla: if it saves a lot of time, I'm ok with making this block React only for now

I think all of the above has priority above something like rich-text captions

Overall, great work so far and this will be an amazing addition to BlockNote!

packages/core/src/BlockNoteEditor.ts Show resolved Hide resolved
packages/core/src/BlockNoteEditor.ts Show resolved Hide resolved
packages/react/src/Image.tsx Outdated Show resolved Hide resolved
packages/react/src/Image.tsx Outdated Show resolved Hide resolved
packages/react/src/Image.tsx Outdated Show resolved Hide resolved
* small fixes

* Updated `uploadFile` JSDoc

* Extracted example file upload function to new file removed it as a default

---------

Co-authored-by: Matthew Lipski <matthewlipski@gmail.com>
# Conflicts:
#	packages/core/src/extensions/Blocks/nodes/BlockContainer.ts
#	packages/core/src/extensions/Blocks/nodes/BlockContent/ListItemBlockContent/NumberedListItemBlockContent/NumberedListItemBlockContent.ts
#	packages/react/src/FormattingToolbar/components/DefaultButtons/ColorStyleButton.tsx
#	packages/react/src/SharedComponents/Toolbar/components/ToolbarDropdown.tsx
#	packages/react/src/SideMenu/components/DragHandleMenu/DefaultButtons/BlockColorsButton.tsx
packages/website/docs/docs/image-toolbar.md Show resolved Hide resolved
@@ -0,0 +1,57 @@
---
Copy link
Collaborator

Choose a reason for hiding this comment

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

My main feedback on the docs is that it's now all centered around the Toolbar. This works for now, but is not very future proof if we soon plan to add:

  • drag and drop: in that case, image / upload functionality is not related to the toolbar anymore, since you can add an image without opening the toolbar
  • support for other files (arbitrary files, or videos)

Worth considering to already take this into account, or to migrate by then

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed, I think when we implement drag and drop or other file types we'll want to move the docs for uploading to a different section, and have the Image Toolbar section reference it. I'm not sure we want to split them yet though, since right now the image toolbar is the only thing that does file uploading.

@matthewlipski matthewlipski merged commit cefbb70 into main Oct 3, 2023
2 checks passed
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