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

Improved removing content after a link element #7668

Merged
merged 9 commits into from
Jul 23, 2020
Merged

Improved removing content after a link element #7668

merged 9 commits into from
Jul 23, 2020

Conversation

pomek
Copy link
Member

@pomek pomek commented Jul 22, 2020

Suggested merge commit message (convention)

Fix (link): When removing the content after a link element, the selection will not preserve the linkHref attribute (and decorators if specified). However, if the selection is placed in the link element (in the middle or at the end), those attributes will be preserved. Closes #7521.

@pomek pomek requested a review from jodator July 22, 2020 11:18
Copy link
Member

@oleq oleq left a comment

Choose a reason for hiding this comment

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

I tested the UX and it looks nice. 

The implementation is crazy :P but AFAIR it has been discussed with @Reinmar so I'm cool with it.

packages/ckeditor5-link/src/linkediting.js Outdated Show resolved Hide resolved
packages/ckeditor5-link/src/linkediting.js Outdated Show resolved Hide resolved
packages/ckeditor5-link/src/linkediting.js Outdated Show resolved Hide resolved
packages/ckeditor5-link/src/linkediting.js Outdated Show resolved Hide resolved
packages/ckeditor5-link/src/linkediting.js Outdated Show resolved Hide resolved
packages/ckeditor5-link/src/linkediting.js Outdated Show resolved Hide resolved
* {@link module:typing/twostepcaretmovement~TwoStepCaretMovement} plugin is activated and
* the selection is inside the link, at the end, the `linkHref` attribute should stay untouched.
*
* The purpose of this action is to allow removing the link label and keep the selection outside the link.
Copy link
Member

Choose a reason for hiding this comment

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

TBH I don't understand the "link label" here. WDYM?

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 meant a text that described the URL <a href="url">link label</a>.

@oleq
Copy link
Member

oleq commented Jul 22, 2020

@ckeditor/qa-team Can you tak a look at this PR?

Copy link
Contributor

@jodator jodator left a comment

Choose a reason for hiding this comment

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

Besides @oleq 's comments I'd change the method name - so cosmetics only. The refactoring done here is nice :)

From the UX perspective I think that we could handle the same way del. I've tested it by setting hasBackspacePressed = true and it worked as expected. In other words the code for handling hasBackspacePressed could be removed to handle both delete directions.

packages/ckeditor5-link/src/linkediting.js Outdated Show resolved Hide resolved
@FilipTokarski
Copy link
Member

Looks ok 👍

@pomek
Copy link
Member Author

pomek commented Jul 22, 2020

@jodator, if I remove del checks, after pressing Del in the following structure:

<paragraph>Foo <$text linkHref="url">Bar</$text>[ ]</paragraph>

link attributes will be removed from the selection as well. Are we fine with that?

@jodator
Copy link
Contributor

jodator commented Jul 23, 2020

link attributes will be removed from the selection as well. Are we fine with that?

AFAICS it would behave the same way as backspace so it is OK for me.

If you see that we can squeeze that change with del quickly then I'd do that. If not it's fine to make that a follow up (#7521 (comment)).

@jodator jodator merged commit c175e1c into master Jul 23, 2020
@jodator jodator deleted the i/7521 branch July 23, 2020 08:38
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.

After backspacing into a link, the caret should still stay outside
4 participants