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

Extend range of drag-and-drop selection to fully selected parents #14891

Merged
merged 12 commits into from
Sep 15, 2023

Conversation

filipsobol
Copy link
Member

@filipsobol filipsobol commented Aug 30, 2023

Suggested merge commit message (convention)

Feature (clipboard): Extend drag-and-drop selection to parent elements when all their children are selected. Closes #14640.

@filipsobol filipsobol changed the title [WIP] Extend range of drag-and-drop selection to fully selected parents Extend range of drag-and-drop selection to fully selected parents Sep 13, 2023
@filipsobol filipsobol marked this pull request as ready for review September 13, 2023 13:48
Copy link
Contributor

@pszczesniak pszczesniak left a comment

Choose a reason for hiding this comment

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

Should it also works when dragging a blockquote which contains a blockquote?

Screen.Recording.2023-09-14.at.08.26.51.mov

Copy link
Contributor

@pszczesniak pszczesniak left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@illia-stv
Copy link
Contributor

illia-stv commented Sep 14, 2023

When you drop content from caption outside widget it still stay there. So we should remember about that and create an issue for this bug.

Manual.Test.-.14.September.2023.mp4

* Because all elements inside the `blockQuote` are selected, the range includes the `blockQuote` too.
* If only first and second paragraphs would be selected, the range would not include it.
*/
function getRangeExtendedToLargestFullySelectedParent( model: Model, elements: Array<Element> ): Range {
Copy link
Contributor

Choose a reason for hiding this comment

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

To the getRangeExtendedToLargestFullySelectedParent we pass blocks, so maybe it could be good name for this argument instead elements, what do you think? To be honest I would call it selectedBlocks.

* Because all elements inside the `blockQuote` are selected, the range includes the `blockQuote` too.
* If only first and second paragraphs would be selected, the range would not include it.
*/
function getRangeExtendedToLargestFullySelectedParent( model: Model, elements: Array<Element> ): Range {
Copy link
Contributor

Choose a reason for hiding this comment

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

This name is too long for me, maybe we could simplify it a little bit to getRangeExtendedToSelectedParent?

Copy link
Member Author

Choose a reason for hiding this comment

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

I renamed it to getRangeIncludingFullySelectedParents as it better describes what this function does.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like it!!! But why parents and not parent?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because this function is recursive, so if first common parent doesn't have any siblings, its parent will also be included in the range and so on.

@illia-stv
Copy link
Contributor

When you select blockquote with only part of paragraph it selects entire paragraph instead.

Manual.Test.-.15.September.2023.mp4

Copy link
Contributor

@illia-stv illia-stv 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 ✅

@filipsobol filipsobol merged commit 5e26217 into master Sep 15, 2023
7 checks passed
@filipsobol filipsobol deleted the ck/14640-blockquote-formatting-is-lost branch September 15, 2023 12:32
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.

[Drag & drop, intro] Blockquote formatting is lost
4 participants