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(ui) : the block can be drag n drop #2285

Open
wants to merge 89 commits into
base: next
Choose a base branch
from
Open

Conversation

robonetphy
Copy link
Member

resolves #838
Also improves #2019

@ilyamore88
Copy link
Member

Hi, @robonetphy.

Scroll feature works only for scrolling to the top, not to the bottom:

Screen.Recording.2023-04-02.at.22.25.16.mov

Copy link
Member

Choose a reason for hiding this comment

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

Can we extract blocks drag'n'drop feature to a separate class?

Copy link
Member

Choose a reason for hiding this comment

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

Editor can trigger events, and that class will subscribe on them

Copy link
Member Author

Choose a reason for hiding this comment

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

we can but the logic of dropZonePosition is kinda distributed will cause problem I guess.

Copy link
Member

Choose a reason for hiding this comment

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

dropZonePosition could stay a property of a Block class, it's ok

Copy link
Member Author

Choose a reason for hiding this comment

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

can you suggest me for which event we need to do this?

Copy link
Member

Choose a reason for hiding this comment

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

I guess all the events you're using for that feature:

EditorDragStart
EditorDragEnd
EditorDragOver (if needed)
EditorDrop

The idea is to make a standalone class with blocks drag'n'drop, that wont extend a __Module, but use Editor.js API instead.

Check how the toolbar/index.ts and Toolbox.ts works. Ui.ts emits the block-hovered event and doing its own logic.

/**
 * Subscribe to the 'block-hovered' event
 */
this.eventsDispatcher.on(this.Editor.UI.events.blockHovered, (data: {block: Block}) => {
  /**
   * Do not move toolbar if Block Settings or Toolbox opened
   */
  if (this.Editor.BlockSettings.opened || this.toolboxInstance.opened) {
    return;
  }

  this.moveAndOpen(data.block);
});

The toolbar/index.ts is still a module, but it would be refactored later. The Toolbox.ts already refactored becaming organized as a standalone class.

this.toolboxInstance = new Toolbox({
      api: this.Editor.API.methods,
      tools: this.Editor.Tools.blockTools,
      i18nLabels: {
        filter: I18n.ui(I18nInternalNS.ui.popover, 'Filter'),
        nothingFound: I18n.ui(I18nInternalNS.ui.popover, 'Nothing found'),
      },
    });

It works only throught the Editor.js API.

Copy link
Member

Choose a reason for hiding this comment

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

You can add some API methods if you need. It would be better if that methods will be abstract and usefull not only for a particular feature, but for other potential cases as well

src/components/modules/dragNDrop.ts Outdated Show resolved Hide resolved
Caret.setToBlock(targetBlock, Caret.positions.END);
}
/**
* @todo -Implement to drop a block before the first block,
Copy link
Member

Choose a reason for hiding this comment

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

Will you do it in this PR or in a separate one?

Copy link
Member Author

Choose a reason for hiding this comment

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

yup

Copy link
Member

Choose a reason for hiding this comment

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

Soo 😄

Copy link
Member

Choose a reason for hiding this comment

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

In a separate?

src/components/modules/dragNDrop.ts Outdated Show resolved Hide resolved

const numberOfBlocks = 3;

for (let i = 0; i < numberOfBlocks; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

Why don't you render the editor just with these blocks?

Comment on lines 196 to 198
expect(blocks[0].textContent).to.eq('Block 1');
expect(blocks[1].textContent).to.eq('Block 2');
expect(blocks[2].textContent).to.eq('Block 0');
Copy link
Member

Choose a reason for hiding this comment

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

Also would be great to test output data from the .save method.

@robonetphy
Copy link
Member Author

Hi, @robonetphy. If I have a tip with a 'Click to tune' message and I start moving a block, there is a part of the tune word:

resolve the problem as @ilyamore88 suggested

@codex-team codex-team deleted a comment from UmangRobro Apr 3, 2023
@codex-team codex-team deleted a comment from UmangRobro Apr 3, 2023
@codex-team codex-team deleted a comment from UmangRobro Apr 3, 2023
@codex-team codex-team deleted a comment from UmangRobro Apr 3, 2023
@robonetphy
Copy link
Member Author

Hi, @robonetphy.

Scroll feature works only for scrolling to the top, not to the bottom:

Pls remove the our fixed toolbar at the bottom of the our example and then it will work

@ilyamore88
Copy link
Member

Hi, @robonetphy. If I have a tip with a 'Click to tune' message and I start moving a block, there is a part of the tune word:

resolve the problem as @ilyamore88 suggested

Hi, this problem is not reproducible anymore, thanks.

@ilyamore88
Copy link
Member

Hi, @robonetphy.
Scroll feature works only for scrolling to the top, not to the bottom:

Pls remove the our fixed toolbar at the bottom of the our example and then it will work

What if a user has a footer on his site?

@robonetphy
Copy link
Member Author

robonetphy commented Apr 3, 2023

Hi, @robonetphy.
Scroll feature works only for scrolling to the top, not to the bottom:

Pls remove the our fixed toolbar at the bottom of the our example and then it will work

What if a user has a footer on his site?

position:fixed will be problem to our every logic don't you think, yup it work wilth footer unless and util it positioned not fixed.

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.

Drag'n'drop
5 participants