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

4715: Autolink feature #7478

Merged
merged 32 commits into from
Jun 24, 2020
Merged

4715: Autolink feature #7478

merged 32 commits into from
Jun 24, 2020

Conversation

jodator
Copy link
Contributor

@jodator jodator commented Jun 22, 2020

Suggested merge commit message (convention)

Feature (link): Added the AutoLink feature which replaces a plain text with a link if typed or pasted content is the link. Closes #4715.


Additional information

  • I'm not sure how verbose the feature guide should be.
  • I've might forget something due to various distractions so sorry if found something serious :(

@jodator
Copy link
Contributor Author

jodator commented Jun 22, 2020

Hey, @ckeditor/qa-team - there's a new feature to check - AutoLink. It has its own manual test and section in Link feature guide.

cc @Reinmar since Olek is out could you take a quick look at the feature guide:

@jodator jodator requested a review from pomek June 22, 2020 13:16
@pomek
Copy link
Member

pomek commented Jun 23, 2020

Test case:

  1. Use all-features sample. Add the AutoLink plugin.
  2. Clear the editor.
  3. Type: Type whatever.
  4. Each word is treated as link.

image

The same result I've got in the autolink MT.

Copy link
Member

@pomek pomek left a comment

Choose a reason for hiding this comment

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

The feature looks good. Unfortunately, it replaces too many words.

packages/ckeditor5-link/src/autolink.js Show resolved Hide resolved
packages/ckeditor5-link/src/autolink.js Outdated Show resolved Hide resolved
packages/ckeditor5-link/tests/autolink.js Show resolved Hide resolved
packages/ckeditor5-link/tests/manual/autolink.js Outdated Show resolved Hide resolved
@pomek
Copy link
Member

pomek commented Jun 23, 2020

From CI:

Checking dependencies...
Found some issue with dependencies.
Missing devDependencies:
- @ckeditor/ckeditor5-code-block

💥 link have a dependency problem 💥

@pomek
Copy link
Member

pomek commented Jun 23, 2020

I've updated the commit/merge message.

jodator and others added 2 commits June 24, 2020 06:38
Co-authored-by: Kamil Piechaczek <pomek@users.noreply.github.com>
@jodator jodator requested a review from pomek June 24, 2020 07:20
@jodator
Copy link
Contributor Author

jodator commented Jun 24, 2020

@pomek I think I've addressed all the issues.

@Mgsy

This comment has been minimized.

@jodator
Copy link
Contributor Author

jodator commented Jun 24, 2020

A text remains as a plain text.

I think that the selection remains after <br> so the second enter is executed on:

https://google.pl<br>
[]

In that case it works OK.

Do you recall a bug with rendering selection after shift+enter? I think that I saw something like that.

@jodator
Copy link
Contributor Author

jodator commented Jun 24, 2020

@Mgsy - I've found something similar: #1576 looks like Chrome bug.

@Mgsy
Copy link
Member

Mgsy commented Jun 24, 2020

I think that the selection remains after <br> so the second enter is executed on:

True, now I see that the selection doesn't change - it stays after <br>. Only the caret renders in incorrect place.

@jodator
Copy link
Contributor Author

jodator commented Jun 24, 2020

Yep, FF works fine:

@jodator
Copy link
Contributor Author

jodator commented Jun 24, 2020

@pomek two more things to add. I've forgot about disabling this.

@jodator
Copy link
Contributor Author

jodator commented Jun 24, 2020

@pomek OK I've fixed the issues with disabling AutoLink feature in code blocks and when using forceDisabled().

@Mgsy

This comment has been minimized.

@jodator
Copy link
Contributor Author

jodator commented Jun 24, 2020

@Mgsy dup of: #7459 ?

@Mgsy
Copy link
Member

Mgsy commented Jun 24, 2020

@Mgsy dup of: #7459 ?

Yes, it looks like this is the same issue :)

@Mgsy
Copy link
Member

Mgsy commented Jun 24, 2020

I've tested it in various browsers and the feature looks good 👍

# Conflicts:
#	packages/ckeditor5-link/package.json
@pomek pomek merged commit c3f3078 into master Jun 24, 2020
@pomek pomek deleted the i/4715 branch June 24, 2020 13:21
@jay2503
Copy link

jay2503 commented Jul 26, 2020

is this feature released with v20.0.0? I could not find autolink.js under source with the latest installation.

@jodator
Copy link
Contributor Author

jodator commented Jul 26, 2020

@jay2503 - it will be available in the upcoming release. If everything goes well it should be released next week.

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.

Implement autolinking feature
4 participants