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

i/4665: Added customization of accept mention keys #9751

Merged
merged 6 commits into from
Aug 2, 2021
Merged

i/4665: Added customization of accept mention keys #9751

merged 6 commits into from
Aug 2, 2021

Conversation

mateuszzagorski
Copy link
Contributor

@mateuszzagorski mateuszzagorski commented May 25, 2021

Suggested merge commit message (convention)

Fix (mention): The mention plugin will now allow users to customize the accept mention keys by adding the commitKeys configuration option. Closes #4665.

MINOR BREAKING CHANGE (mention): It is now possible to create a custom array of keyCodes that will be used by the editor to commit mentions. If no custom configuration has been added the default one (enter + tab) will be used as it was before.


Additional information

Added manual test, covered with unit tests, and updated API docs with an example of how to use the applied changes.

@mateuszzagorski mateuszzagorski requested a review from mlewand May 25, 2021 12:30
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.

  • This PR allows integrators to take back the control over Enter and Tab keys when browsing mentions and execute their default actions (new line, focus navigation) instead of committing a mention, which is fine I guess.

  • This PR also allows integrators to use custom keys to commit mentions (such as only Enter but not Tab or "A"), which is also fine.

  • It does not solve the problem of integrators who have mentions such as

I'm not sure this last scenario was really considered in this PR so let's talk it over and define the scope of this PR @mlewand @mateuszzagorski? FYI I'm OK if the last scenario is handled separately because what we have here brings value to the project and we can always iterate.

packages/ckeditor5-mention/src/mention.js Outdated Show resolved Hide resolved
packages/ckeditor5-mention/src/mention.js Outdated Show resolved Hide resolved
packages/ckeditor5-mention/src/mention.js Outdated Show resolved Hide resolved
packages/ckeditor5-mention/src/mention.js Outdated Show resolved Hide resolved
packages/ckeditor5-mention/src/mentionui.js Outdated Show resolved Hide resolved
@@ -1549,7 +1549,7 @@ describe( 'MentionUI', () => {
};

const keyUpEvtData = {
keyCode: keyCodes.arrowdown,
keyCode: keyCodes.arrowup,
Copy link
Member

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have a keyDownEvtData above that's using keyCodes.arrowdown, and this one is called keyUpEvtData - and it looked like it should've been using the keyCodes.arrowup as it makes more sense.

packages/ckeditor5-mention/tests/mentionui.js Outdated Show resolved Hide resolved
packages/ckeditor5-mention/src/mention.js Show resolved Hide resolved
@mateuszzagorski mateuszzagorski requested a review from oleq July 28, 2021 11:33
@mlewand
Copy link
Contributor

mlewand commented Jul 29, 2021

Regarding the scope: related issue focuses on key handling and I see that comments added couple of months later diverged into handling space in mentions (which is #9741).

@mateuszzagorski @oleq let's stick to the original scope of this PR as we're running low on time now.

@oleq oleq merged commit 9fa1052 into master Aug 2, 2021
@oleq oleq deleted the i/4665 branch August 2, 2021 08:16
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.

Customize keys to accept mention
3 participants