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 issue of duplicated blocks upon pasting an image into the Slate E… #5818

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

aryan7081
Copy link

Fixes Issue

Inside the onImageLoad function in withDeserializers extension of Volto Slate.
uploadContent adds the block using Redux and FormData, but the .then promise callback of uploadContent also adds a new Node in the Slate Editor using Transforms.insertNodes(editor, image); which eventually leads to adding multiple images in the editor.

Copy link

netlify bot commented Mar 1, 2024

Deploy Preview for volto canceled.

Name Link
🔨 Latest commit 138a634
🔍 Latest deploy log https://app.netlify.com/sites/volto/deploys/65e9ee11d325e500086f471f

Copy link

netlify bot commented Mar 1, 2024

Deploy Preview for plone-components canceled.

Name Link
🔨 Latest commit f98ddb0
🔍 Latest deploy log https://app.netlify.com/sites/plone-components/deploys/66545d526058770008f8fd43

packages/volto-slate/news/5818.bugfix Outdated Show resolved Hide resolved
Co-authored-by: Steve Piercy <web@stevepiercy.com>
Copy link
Collaborator

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

News looks good, PR needs review by a maintainer.

@stevepiercy
Copy link
Collaborator

@plone/volto-team review requested.

Copy link
Collaborator

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

Change log is OK. Code needs review by a maintainer. @plone/volto-team

@ichim-david ichim-david self-requested a review March 2, 2024 08:24
Copy link
Sponsor Member

@ichim-david ichim-david left a comment

Choose a reason for hiding this comment

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

@aryan7081 I wonder how did you test that this code change fixes the issue?
I will attach a video where I record testing your fix and the issue is still present
exactly as it was flagged into the issue so I don't understand what made you belive that this issue would be fixed by your change.

slate-image-pasting.mp4

If you still want to try to work on this issue, your pull request needs to contain
a cypress test that uploads an image, you copy the image to the clipboard and you paste it in a page as you can see from this video:

copy-uploaded-image.mp4

EDIT:
Then your test should show that when you paste the image inside the empty slate block, the image is pasted inside that
block instead of leaving that initial slate block empty.

@ichim-david
Copy link
Sponsor Member

ichim-david commented Mar 2, 2024

@aryan7081 A couple of extra context as upon further testing I noticed the following:

  1. copy and pasting an image from the website or from the internet results in the correct behavior regardless of your change
  2. The code that you disabled indeed prevents 2 images from being uploaded when pasting from the os (MAC OS Sonoma in my case)
  3. That is not enough to prevent the slate empty block from being added or from the image being inserted in the first slate block when pasting from the os clipboard for an image copied from Finder app.
    It should be tested also in other os systems such as Linux or Windows

@aryan7081
Copy link
Author

@ichim-david Sorry for the delay. Working on this and adding tests as well. Can I test this on linux and windows vm?

@ichim-david
Copy link
Sponsor Member

@ichim-david Sorry for the delay. Working on this and adding tests as well. Can I test this on linux and windows vm?

@aryan7081 do the minimum test first on whatever platform you're working on and if you can reproduce the issue there then try to fix it, there is no point in doing cross-testing until necessary.

@aryan7081
Copy link
Author

aryan7081 commented Mar 7, 2024

The createImageBlock function within packages/volto-slate/src/utils/volto-blocks.js currently utilizes addBlock to insert a new block at the index position plus one. However, the addBlock function adds multiple blocks: one for the block being added and another empty block if the type is not 'slate'.
To address this, I've attempted to resolve the issue by modifying the createImageBlock function as follows:

const currBlockId = properties.blocks_layout.items[index];
const currBlockHasValue = blockHasValue(properties.blocks[currBlockId]);
let id, newFormData;
if (currBlockHasValue) {
  [id, newFormData] = addBlock(properties, 'image', index + 1);
  newFormData = changeBlock(newFormData, id, { '@type': 'image', url });
} else {
  [id, newFormData] = insertBlock(properties, currBlockId, { '@type': 'image', url });
}

This change ensures that a block is inserted at the (index - 1) position if the current(index) block is empty. Otherwise, it proceeds to create another block as usual.
Please let me know your thoughts or any further adjustments needed.

Will push cypress tests once this approach is validated.

Screen.Recording.2024-03-07.at.9.01.39.PM.MOV

@stevepiercy
Copy link
Collaborator

@aryan7081 please run code quality checks locally before pushing commits. See Linting.

However our documentation lacks the specific commands that need to be run, so I don't blame you for not running undocumented commands. See #5341

The CI jobs call https://github.com/plone/volto/blob/main/.github/workflows/code-analysis.yml. You can look at it for the commands that do not pass the checks, and try running them locally.

@aryan7081
Copy link
Author

@stevepiercy Working on cypress tests.

Copy link
Member

@sneridagh sneridagh left a comment

Choose a reason for hiding this comment

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

@aryan7081 I tried it out, and it does work well indeed.

We would need the tests if possible. We improved the documentation lately:
https://6.docs.plone.org/volto/contributing/acceptance-tests.html

Also, the code analysis checks are failing.

Could you imagine in work on this?

@sneridagh sneridagh added this to the 18.x.x milestone May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
4 participants