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

Add validation to the link form #15633

Merged
merged 15 commits into from
Jan 8, 2024

Conversation

filipsobol
Copy link
Member

@filipsobol filipsobol commented Jan 4, 2024

Suggested merge commit message (convention)

MINOR BREAKING CHANGE (link): Add validation to the URL field to disallow empty URL by default. Closes #12501.


Additional information

For example – encountered issues, assumptions you had to make, other affected tickets, etc.

@Witoso
Copy link
Member

Witoso commented Jan 5, 2024

Update guide mention needed in this (how to restore previous behavior).

Copy link
Contributor

@mmotyczynska mmotyczynska left a comment

Choose a reason for hiding this comment

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

Approved ✅
Just a few typos to fix.

I like that you added InputUrlView class 👍

We must not forget about #15633 (comment). Do we have a follow-up issue or have we mentioned it somewhere?

packages/ckeditor5-ui/tests/inputurl/inputurlview.js Outdated Show resolved Hide resolved
packages/ckeditor5-ui/src/labeledfield/utils.ts Outdated Show resolved Hide resolved
@filipsobol filipsobol marked this pull request as draft January 5, 2024 14:44
@Witoso
Copy link
Member

Witoso commented Jan 5, 2024

Let's reject/rethink IMHO. I want to mention that it is possible to add (relative) text into the link field right now (which will result in a relative link). As far as I can see, this behavior changed, as it's not being treated as a valid URL by the URL input field.

PR:

Current docs:

@Witoso
Copy link
Member

Witoso commented Jan 5, 2024

The URL input itself looks great, I even wondered if it could be decided dynamically which one is used (when defaultProtocol config is provided use URL field). But then protocol can be anything, and it may require a more advanced logic (pattern validation).

Copy link
Contributor

@gorzelinski gorzelinski left a comment

Choose a reason for hiding this comment

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

I only spotted some minor mistakes.

packages/ckeditor5-link/src/linkconfig.ts Outdated Show resolved Hide resolved
docs/framework/architecture/ui-components.md Outdated Show resolved Hide resolved
docs/framework/architecture/ui-components.md Outdated Show resolved Hide resolved
@filipsobol filipsobol marked this pull request as ready for review January 8, 2024 09:47
Copy link
Contributor

@mmotyczynska mmotyczynska left a comment

Choose a reason for hiding this comment

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

Some more typos to be fixed.

Generally looks good 👍 but I've found one case where the button is active even though the input text is empty. But this doesn't add an empty link to the editor because the link text is empty. IMO it's worth fixing so that the UI is consistent.

Screen.Recording.2024-01-08.at.11.41.27.mov

docs/framework/architecture/ui-components.md Outdated Show resolved Hide resolved
docs/framework/architecture/ui-components.md Outdated Show resolved Hide resolved
docs/framework/architecture/ui-components.md Outdated Show resolved Hide resolved
Co-authored-by: Marta Motyczynska <[email protected]>
…tion-to-the-link-form' into ck/12501-add-validation-to-the-link-form
@filipsobol
Copy link
Member Author

Generally looks good 👍 but I've found one case where the button is active even though the input text is empty. But this doesn't add an empty link to the editor because the link text is empty. IMO it's worth fixing so that the UI is consistent.

Fixed

Copy link
Contributor

@gorzelinski gorzelinski left a comment

Choose a reason for hiding this comment

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

Now, it looks good to me!

@filipsobol filipsobol merged commit 811809b into master Jan 8, 2024
7 checks passed
@filipsobol filipsobol deleted the ck/12501-add-validation-to-the-link-form branch January 8, 2024 14:24
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.

Empty value is getting added as a link to a text.
5 participants