Add New Feature #21

Closed

s524797336 wants to merge 4 commits from master into master

pull from: master

merge into: prosemirror:master

prosemirror:main

Conversation 18 Commits 4 Files changed 1 +87 -8

s524797336 commented 2024-11-20 12:36:53 +01:00 (Migrated from github.com)

Copy link

1. Added some options to keep the cursor element from being removed during drag operations, ensuring the continuity of animations like transition.
2. Added the disableDropCursor type to prosemirror-model to provide code hints when writing NodeSpec.
3. Add cursor dropEffect when disableDropCursor option triggered.

1. Added some options to keep the `cursor element` from being removed during drag operations, ensuring the continuity of animations like `transition`. 2. Added the `disableDropCursor` type to `prosemirror-model` to provide code hints when writing `NodeSpec`. 3. Add cursor `dropEffect` when `disableDropCursor` option triggered.

marijnh commented 2024-11-26 12:34:01 +01:00 (Migrated from github.com)

Copy link

How about making this the default behavior, instead of an option, and using a fixed class name, so that no configuration is necessary?

How about making this the default behavior, instead of an option, and using a fixed class name, so that no configuration is necessary?

s524797336 commented 2024-11-26 14:21:47 +01:00 (Migrated from github.com)

Copy link

How about making this the default behavior, instead of an option, and using a fixed class name, so that no configuration is necessary?

I personally think it's fine, but I'm worried that others might not need this behavior. Adding a default class is acceptable. If it's set as the default behavior, I think at least an option should be added, which can default to true. If you agree with me, I'll make the changes. Or do you think anything else is needed?

> How about making this the default behavior, instead of an option, and using a fixed class name, so that no configuration is necessary? I personally think it's fine, but I'm worried that others might not need this behavior. Adding a default class is acceptable. If it's set as the default behavior, I think at least an option should be added, which can default to true. If you agree with me, I'll make the changes. Or do you think anything else is needed?

marijnh commented 2024-11-26 14:34:08 +01:00 (Migrated from github.com)

Copy link

In what way can this cause problems for existing users? The hiding class would be styled as display: none in the styles, which would in almost all circumstances be indistinguishable from being removed from the DOM.

In what way can this cause problems for existing users? The hiding class would be styled as `display: none` in the styles, which would in almost all circumstances be indistinguishable from being removed from the DOM.

s524797336 commented 2024-11-26 14:47:27 +01:00 (Migrated from github.com)

Copy link

In what way can this cause problems for existing users? The hiding class would be styled as display: none in the styles, which would in almost all circumstances be indistinguishable from being removed from the DOM.

I noticed that the Milkdown editor already uses transition animations, following the old behavior. During dragging, the animation breaks. Perhaps they adjusted the animation style based on this breaks. Changing this would make the animation continuous, but it might not be what they intended.

> In what way can this cause problems for existing users? The hiding class would be styled as `display: none` in the styles, which would in almost all circumstances be indistinguishable from being removed from the DOM. I noticed that the Milkdown editor already uses transition animations, following the old behavior. During dragging, the animation breaks. Perhaps they adjusted the animation style based on this breaks. Changing this would make the animation continuous, but it might not be what they intended.

s524797336 commented 2024-11-26 15:02:47 +01:00 (Migrated from github.com)

Copy link

I'm ok with both, your repo, your choice.

I'm ok with both, your repo, your choice.

marijnh commented 2024-11-27 14:58:19 +01:00 (Migrated from github.com)

Copy link

Oh, on closer look it seems the package does not use any actual CSS at the moment, so there's no place to define the class that'd hide the cursor. Would a solution that exposes an inactiveClass option and just sets display: none when none is defined work for you?

Oh, on closer look it seems the package does not use any actual CSS at the moment, so there's no place to define the class that'd hide the cursor. Would a solution that exposes an `inactiveClass` option and just sets `display: none` when none is defined work for you?

s524797336 commented 2024-11-27 15:41:55 +01:00 (Migrated from github.com)

Copy link

Oh, on closer look it seems the package does not use any actual CSS at the moment, so there's no place to define the class that'd hide the cursor. Would a solution that exposes an inactiveClass option and just sets display: none when none is defined work for you?

Of course

> Oh, on closer look it seems the package does not use any actual CSS at the moment, so there's no place to define the class that'd hide the cursor. Would a solution that exposes an `inactiveClass` option and just sets `display: none` when none is defined work for you? Of course

s524797336 commented 2024-11-27 16:35:00 +01:00 (Migrated from github.com)

Copy link

In the latest commit, I added a shouldCalcDropPoint option to sync with my copy/paste plugin. Sometimes, I don't want to calculate the drop point, and my editor forced a change in the code format. I don't really want to revert it, so I hope you don't mind.

In the latest commit, I added a shouldCalcDropPoint option to sync with my copy/paste plugin. Sometimes, I don't want to calculate the drop point, and my editor forced a change in the code format. I don't really want to revert it, so I hope you don't mind.

marijnh commented 2024-11-27 17:10:53 +01:00 (Migrated from github.com)

Copy link

I absolutely do mind. PRs with superfluous changes in them are not welcome.

Looking at the code, I'm wondering what kind of situation, in the current code, causes the cursor to be hidden when a drag is still in progress. It seems setCursor(null) is only called in response to dragend/dragleave/drop events.

I absolutely do mind. PRs with superfluous changes in them are not welcome. Looking at the code, I'm wondering what kind of situation, in the current code, causes the cursor to be hidden when a drag is still in progress. It seems `setCursor(null)` is only called in response to `dragend`/`dragleave`/`drop` events.

s524797336 commented 2024-11-27 17:48:24 +01:00 (Migrated from github.com)

Copy link

I absolutely do mind. PRs with superfluous changes in them are not welcome.

Ok, i change back the code format

Looking at the code, I'm wondering what kind of situation, in the current code, causes the cursor to be hidden when a drag is still in progress. It seems setCursor(null) is only called in response to dragend/dragleave/drop events.

I’m not sure I understand what you mean. Are you saying that the original code doesn’t cause the issue I mentioned, or that the code I submitted is causing the cursor to be hidden during the drag?

> I absolutely do mind. PRs with superfluous changes in them are not welcome. Ok, i change back the code format > Looking at the code, I'm wondering what kind of situation, in the current code, causes the cursor to be hidden when a drag is still in progress. It seems setCursor(null) is only called in response to dragend/dragleave/drop events. I’m not sure I understand what you mean. Are you saying that the original code doesn’t cause the issue I mentioned, or that the code I submitted is causing the cursor to be hidden during the drag?

marijnh commented 2024-11-28 10:32:54 +01:00 (Migrated from github.com)

Copy link

Are you saying that the original code doesn’t cause the issue I mentioned

I'm sure there was a reason you started on this patch, but I don't see any path in the current code that'll call setCursor(null) while a drag is still ongoing, so I'd like to hear more about what situation you are addressing here.

> Are you saying that the original code doesn’t cause the issue I mentioned I'm sure there was a reason you started on this patch, but I don't see any path in the current code that'll call `setCursor(null)` while a drag is still ongoing, so I'd like to hear more about what situation you are addressing here.

s524797336 commented 2024-11-28 11:05:35 +01:00 (Migrated from github.com)

Copy link

I'm sure there was a reason you started on this patch, but I don't see any path in the current code that'll call setCursor(null) while a drag is still ongoing, so I'd like to hear more about what situation you are addressing here.

First, you can see that the drag between the two paragraphs was interrupted because the dragleave event was triggered.

Second, when a NodeSpect's disableDropCursor is set to true, the dragleave event is also triggered.

After change

My English isn't very good. If I say something unclear, feel free to ask me anytime.

> I'm sure there was a reason you started on this patch, but I don't see any path in the current code that'll call setCursor(null) while a drag is still ongoing, so I'd like to hear more about what situation you are addressing here. First, you can see that the drag between the two paragraphs was interrupted because the `dragleave` event was triggered.  Second, when a `NodeSpect`'s `disableDropCursor` is set to true, the `dragleave` event is also triggered.  After change  My English isn't very good. If I say something unclear, feel free to ask me anytime.

marijnh commented 2024-11-28 11:11:44 +01:00 (Migrated from github.com)

Copy link

I see, dragleave is also fired when the drag moves from a node to a descendent. It looks like we could just remove the event.target == this.editorView.dom || part from the dragleave handler, to avoid this unnecessary hiding.

I see, `dragleave` is also fired when the drag moves from a node to a descendent. It looks like we could just remove the `event.target == this.editorView.dom ||` part from the `dragleave` handler, to avoid this unnecessary hiding.

s524797336 commented 2024-11-28 11:34:26 +01:00 (Migrated from github.com)

Copy link

I see, dragleave is also fired when the drag moves from a node to a descendent. It looks like we could just remove the event.target == this.editorView.dom || part from the dragleave handler, to avoid this unnecessary hiding.

I applied this change and found that the only difference compared to what I changed is when dragging outside the editor, which is acceptable. So, I will revert this part of the code. Can the rest of the changes be merged?

> I see, `dragleave` is also fired when the drag moves from a node to a descendent. It looks like we could just remove the `event.target == this.editorView.dom ||` part from the `dragleave` handler, to avoid this unnecessary hiding. I applied this change and found that the only difference compared to what I changed is when dragging outside the editor, which is acceptable. So, I will revert this part of the code. Can the rest of the changes be merged?

marijnh commented 2024-11-28 11:36:31 +01:00 (Migrated from github.com)

Copy link

With my change, is there any situation left where the cursor is hidden while dragging inside the editor? I.e. are your changes still necessary?

With my change, is there any situation left where the cursor is hidden while dragging inside the editor? I.e. are your changes still necessary?

s524797336 commented 2024-11-28 11:39:20 +01:00 (Migrated from github.com)

Copy link

With my change, is there any situation left where the cursor is hidden while dragging inside the editor? I.e. are your changes still necessary?

My change still contain two more changes

1. Added the disableDropCursor type to prosemirror-model to provide code hints when writing NodeSpec.
2. Add cursor dropEffect when disableDropCursor option triggered.

> With my change, is there any situation left where the cursor is hidden while dragging inside the editor? I.e. are your changes still necessary? My change still contain two more changes 1. Added the `disableDropCursor` type to prosemirror-model to provide code hints when writing NodeSpec. 2. Add cursor `dropEffect` when `disableDropCursor` option triggered.

s524797336 commented 2024-11-28 11:41:53 +01:00 (Migrated from github.com)

Copy link

Oh, also a inactive class name and shouldCalcDropPoint option i mention before

Oh, also a inactive class name and `shouldCalcDropPoint` option i mention before

marijnh commented 2024-11-28 11:51:31 +01:00 (Migrated from github.com)

Copy link

I think you may be better off forking this extension—I don't really want to add this kind of controls, which would have to be carefully synchronized with a drop handler to make sure the cursor and the actual drop behavior are aligned, to this code.

I've merged the disableDropCursor declaration in b4de30c

I think you may be better off forking this extension—I don't really want to add this kind of controls, which would have to be carefully synchronized with a drop handler to make sure the cursor and the actual drop behavior are aligned, to this code. I've merged the `disableDropCursor` declaration in b4de30c

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

prosemirror/prosemirror-dropcursor!21

No description provided.