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

Change -engine/bindTwoStepCaretToAttribute util to -typing/TwoStepCaretMovement plugin. #7506

Merged
merged 11 commits into from
Jul 1, 2020

Conversation

tomalec
Copy link
Contributor

@tomalec tomalec commented Jun 26, 2020

Suggested merge commit message (convention)

Other (engine, typing): The engine's util bindTwoStepCaretToAttribute is now typing's plugin TwoStepCaretMovement . Closes #7444.

MINOR BREAKING CHANGE (engine, typing): Package and API for bindTwoStepCaretToAttribute(…) changed to editor.plugins.get( TwoStepCaretMovement ).registerAttribute( 'a' )


Additional information

I'd also move highlighting code a part of this plugin (currently hard-coded in Link), but I'd do that in a separate PR, for an easier review.

I will add more tests for multi-attribute scenarios, in a PR that adds 2SCM to the Code plugin.

I was thinking about adding the unregisterAttribute method for cleanup and scenarios when somebody would like to disable 2SCM for Link. Alternatively, we could make a more breaking change and disable 2SCM for Link and require setting it up in config, by just adding 2SCM plugin explicitly.

@tomalec tomalec marked this pull request as ready for review June 26, 2020 09:27
@jodator jodator self-requested a review June 30, 2020 09:34
@jodator jodator self-assigned this Jun 30, 2020
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.

For the refactor there are mostly cosmetics to fix.

It's not entirely clear for me if this should be handled in this PR:

We also agreed, there should be only 2 movements required for multiple attributes ending in the same position. E.g. "some linking code[]" + ←←, results in "some linking code[]". Meaning stepping inside steps into the deepest attribute at given position. It is to minimize the overhead of additional s needed for navigation, simplify the behavior and implementation, especially given, looking at the model with a flat structure of attributes, it's hard to identify the depth of nesting.

I think not - so I would see #7444 to be converted to an epic or have a clear follow up so the scope of the PR is clear.

I've found those issues, but it might be related to a work in progress:

a) ticket 1301 manual tests is not working for me

b) two strikes required to move

packages/ckeditor5-typing/src/twostepcaretmovement.js Outdated Show resolved Hide resolved
packages/ckeditor5-typing/src/twostepcaretmovement.js Outdated Show resolved Hide resolved
Comment on lines 134 to 137
// Do nothing if the plugin is disabled.
// if ( !this.isEnabled ) {
// return;
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

Another lef-over. This one makes me think if we need to enable/disable this plugin. On the first thought I think not - as this is purely navigational plugin but I don't have a strong opinion here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so as well, but I'd like to do that in the next PR, to limit functional changes in this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment removed.

}
export default TwoStepCaretMovement;
Copy link
Contributor

Choose a reason for hiding this comment

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

We tend to keep it together with class declaration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But then you do not have the named export, which is usually my personal preference, but if the convention is to always use default imports, then, I'll change it.

Copy link
Contributor

Choose a reason for hiding this comment

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

The convention is:

export default class TwoStepCareMovement { ... }

not the named export.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed

Co-authored-by: Maciej <jodator@jodator.net>
@tomalec
Copy link
Contributor Author

tomalec commented Jul 1, 2020

It's not entirely clear for me if this should be handled in this PR:

We also agreed, there should be only 2 movements required for multiple attributes

I'd cover it in the next PR, this one is only to refactor, without changing functionality, except making it a plugin.


a) ticket 1301 manual tests is not working for me

Checking...

b) two strikes required to move

That's 0., isn't it? If so, I'll cover that in the next PR

Co-authored-by: Maciej <jodator@jodator.net>
@tomalec
Copy link
Contributor Author

tomalec commented Jul 1, 2020

I reproduced a)
It's FF issue, that also happens on master - I'd consider it out of the scope of this PR.

@pomek
Copy link
Member

pomek commented Jul 1, 2020

MINOR BREAKING CHANGE (engine, typing): Package and API for bindTwoStepCaretToAttribute(…) changed to editor.plugins.get( TwoStepCaretMovement ).registerAttribute( 'a' )

The changelog generator will handle only the first listed package in the scoped BC.

@jodator jodator merged commit d40bd58 into master Jul 1, 2020
@jodator jodator deleted the i/7444-2SCM-refactor branch July 1, 2020 12:47
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.

Move 2-step caret movement to -typing and refactor
3 participants