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

Remove the "unlink" button from the toolbar #4751

Closed
fredck opened this issue Oct 11, 2016 · 19 comments · Fixed by ckeditor/ckeditor5-link#148
Closed

Remove the "unlink" button from the toolbar #4751

fredck opened this issue Oct 11, 2016 · 19 comments · Fixed by ckeditor/ckeditor5-link#148
Assignees
Labels
package:link type:task This issue reports a chore (non-production change) and other types of "todos".
Milestone

Comments

@fredck
Copy link
Contributor

fredck commented Oct 11, 2016

The unlinking feature is already available in the link editing ballon. This is great, because we can have one less button in the toolbar. The less, the better.

@Reinmar
Copy link
Member

Reinmar commented Oct 11, 2016

I totally agree, but it looks like a ticket for a specific usage. E.g. ckeditor5.gihtub.io. Or do you mean to remove the button totally?

@fredck
Copy link
Contributor Author

fredck commented Oct 11, 2016

Yes, I would remove it totally. It can be brought back later on as an optional feature, if necessary, but let's wait and see if it makes sense.

@oskarwrobel
Copy link
Contributor

@Reinmar
Copy link
Member

Reinmar commented Oct 11, 2016

Yes, I would remove it totally. It can be brought back later on as an optional feature, if necessary, but let's wait and see if it makes sense

It may make sense for one thing – you could be able to do ctrl+a and unlink everything. Quite an edge case though.

@fredck
Copy link
Contributor Author

fredck commented Oct 11, 2016

It is already optional https://github.com/ckeditor/ckeditor5-link/blob/master/tests/manual/link.js#L12

Well, everything that goes in the toolbar is optional... but not really:
https://github.com/ckeditor/ckeditor5-link/blob/master/src/link.js#L88-L117

The above code is always executed:
https://github.com/ckeditor/ckeditor5-link/blob/master/src/link.js#L52

Which means that, although we don't want to have it by default, we'll have to keep maintaining it.

If we would ever bring it, it should be "really" optional, like having to load additional features or modules to have it available.

Ofc, we may be wrong here and have to revert the code cleanup related to that. Still I would prefer to not have such code, because later on we'll never be able to remove it.

@fredck
Copy link
Contributor Author

fredck commented Oct 11, 2016

It may make sense for one thing – you could be able to do ctrl+a and unlink everything. Quite an edge case though.

Indeed an edge case. In any case, I would predict a keystroke combination to execute unlink.

@Reinmar
Copy link
Member

Reinmar commented Oct 11, 2016

If we would ever bring it, it should be "really" optional, like having to load additional features or modules to have it available.

+1

@Reinmar
Copy link
Member

Reinmar commented Oct 29, 2016

I've just realised one thing. Since you cannot escape link when typing at the end of it, you'll often need to remove link from a piece of a text. This is pretty cumbersome now:

screen shot 2016-10-29 at 13 18 14

As you can see, there's no link balloon in this case. You can either click the unlink button in the toolbar or open the link balloon and remove the link from there but this is counterintuitive.

So, I'm a bit unsure now about removing the button. The unlink option must be better exposed and/or we must work on escaping link at its right boundary (e.g. implement Firefox's left/right arrow key behaviour).

@fredck
Copy link
Contributor Author

fredck commented Nov 2, 2016

I would, in any case, have no button at the first release. It's easy to bring it in later on.

My point is that, although cumbersome, users will be able to fix the link somehow. Hopefully, this is not an extremely common situation and the benefits of a less bloated toolbar will overcome the disadvantages caused by it.

@Reinmar
Copy link
Member

Reinmar commented Apr 13, 2017

The unlink button was mentioned in a Twitter discussion (https://twitter.com/ckeditor/status/852077313575583744):

image

I'm curious for more opinions on how important is the ability to remove a couple of links at once.

@oleq
Copy link
Member

oleq commented Apr 18, 2017

@Reinmar It could be a problem if we don't provide "Remove formatting" feature. You may want to paste a lengthy chunk of text from the clipboard and use it as a quote but there's a lot of formatting you'd rather see removed because it obscures the meaning of the text. But it still passes the filtering and it's a valid content in the editor.

ATM you can do CTRL+A + 2 * CTRL+B to remove all bolds, italics, etc. but once we get rid of the "Unlink" button, you will need to spend a lot of time to clean-up the content from links. Imagine it was a paragraph from the Wikipedia where almost every single word is a link.

Alternatively... we can provide some fancy shortcut like CTRL+SHIFT+V to paste plain text. Then, we can get rid of "Unlink" without any issue, I think.

@Reinmar
Copy link
Member

Reinmar commented Jul 5, 2017

Let's have a final decision:

@oskarwrobel
Copy link
Contributor

Unlink button in toolbar might also help when you want to remove link which is split by block elements.

jul-06-2017 13-07-53

BTW BPV position is very strange when is attached to the multi line selection.

@oleq
Copy link
Member

oleq commented Jul 6, 2017

BTW BPV position is very strange when is attached to the multi line selection.

It could improve after https://github.com/ckeditor/ckeditor5-utils/issues/168.

@oleq
Copy link
Member

oleq commented Jul 6, 2017

Extend its functionality to strip all links in the selection (as described in #52 (comment)).

@Reinmar: Doesn't it work like this already?

@oskarwrobel
Copy link
Contributor

Unlink button is disabled when selection contains more than link elements.

@oleq
Copy link
Member

oleq commented Jul 6, 2017

So it seems some things have changed since I posted this https://twitter.com/anowodzinski/status/852116748518252544.

I'm for moving the unlink button to the balloon panel then. In the future, we could offer it in the toolbar as an item of a stacked button (drop-button with an arrow), cleaning all links in the selection.

So what I think we should do now:

  1. Remove the unlink button from the toolbar.
  2. Implement the unlink button in the balloon.
  3. Make sure it cleans all the links in the selection so when we add it back to the toolbar in a space–smart way, it's ready to use.

Future:

  1. Implement a UI component grouping the buttons in the form of a dropdown
    image

  2. Put unlink button there.

@Reinmar
Copy link
Member

Reinmar commented Jul 6, 2017

Implement the unlink button in the balloon.

It's already there. You mean – enable it by default?

Make sure it cleans all the links in the selection so when we add it back to the toolbar in a space–smart way, it's ready to use.

Not necessary now.

I'd do the simplest thing for now and simply remove it from the toolbar and component factory. The rest can be moved to a followup. It's not very intuitive for the unlink button in the link balloon to be on when you're not in a link yet anyway, so this wouldn't be a perfect solution either.

@oleq
Copy link
Member

oleq commented Jul 6, 2017

It's already there. You mean – enable it by default?

I suppose I should see my eye doctor 😉. No, I'd leave it as it is, visible only when editing existing link.

I'd do the simplest thing for now and simply remove it from the toolbar and component factory. The rest can be moved to a followup. It's not very intuitive for the unlink button in the link balloon to be on when you're not in a link yet anyway, so this wouldn't be a perfect solution either.

👍

@oleq oleq self-assigned this Jul 18, 2017
oleq referenced this issue in ckeditor/ckeditor5-block-quote Aug 21, 2017
oleq referenced this issue in ckeditor/ckeditor5-core Aug 21, 2017
oleq referenced this issue in ckeditor/ckeditor5-editor-inline Aug 21, 2017
oleq referenced this issue in ckeditor/ckeditor5-essentials Aug 21, 2017
oleq referenced this issue in ckeditor/ckeditor5-ui Aug 21, 2017
Reinmar referenced this issue in ckeditor/ckeditor5-block-quote Aug 22, 2017
Tests: Removed the Unlink button from the toolbar (see ckeditor/ckeditor5-link#52).
Reinmar referenced this issue in ckeditor/ckeditor5-build-classic Aug 22, 2017
Tests: Removed the Unlink button from the toolbar (see ckeditor/ckeditor5-link#52).
Reinmar referenced this issue in ckeditor/ckeditor5-core Aug 22, 2017
Tests: Removed the Unlink button from the toolbar (see ckeditor/ckeditor5-link#52).
Reinmar referenced this issue in ckeditor/ckeditor5-editor-inline Aug 22, 2017
Tests: Removed the Unlink button from the toolbar (see ckeditor/ckeditor5-link#52).
Reinmar referenced this issue in ckeditor/ckeditor5-heading Aug 22, 2017
Reinmar referenced this issue in ckeditor/ckeditor5-heading Aug 22, 2017
Tests: Removed the Unlink button from the toolbar (see ckeditor/ckeditor5-link#52).
Reinmar referenced this issue in ckeditor/ckeditor5-essentials Aug 22, 2017
Tests: Removed the Unlink button from the toolbar (see ckeditor/ckeditor5-link#52).
Reinmar referenced this issue in ckeditor/ckeditor5-ui Aug 22, 2017
Tests: Removed the Unlink button from the toolbar (see ckeditor/ckeditor5-link#52).
Reinmar referenced this issue in ckeditor/ckeditor5-link Aug 22, 2017
Other: Removed the "Unlink" button. Closes #52.

See https://github.com/ckeditor/ckeditor5-link/issues/31#issuecomment-316992952 and https://github.com/ckeditor/ckeditor5-link/issues/149 for plans how unlinking will be exposed in the future.

BREAKING CHANGES: The `unlink'` UI component was removed from the component factory.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-link Oct 9, 2019
@mlewand mlewand added this to the iteration 11 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:task This issue reports a chore (non-production change) and other types of "todos". package:link labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:link type:task This issue reports a chore (non-production change) and other types of "todos".
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants