Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

T/208: TextTransformation should only consider typing changes #213

Merged
merged 12 commits into from Jul 24, 2019
Merged

Conversation

jodator
Copy link
Contributor

@jodator jodator commented Jul 19, 2019

Suggested merge commit message (convention)

Feature: Introduce Input.isInput() method. Closes ckeditor/ckeditor5#3158. Also introduces a fix for the TextTransformation feature - it willl only consider typing changes. Closes ckeditor/ckeditor5#3153.


Additional information

@jodator jodator requested a review from scofalik July 19, 2019 14:12
@coveralls
Copy link

coveralls commented Jul 19, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 2e23a1c on t/208 into 6ef7d47 on master.

@scofalik
Copy link
Contributor

Generally, I dislike heuristics like the one that checks if typing happened. For now, I am okay merging this, but I will make a follow-up in typing to solve the problem of recognizing if typing happened. This is not the first case where this pops up and where it would be useful to have this kind of check.

@scofalik
Copy link
Contributor

https://github.com/ckeditor/ckeditor5-typing/issues/214

@jodator
Copy link
Contributor Author

jodator commented Jul 22, 2019

@scofalik I had other heuristics that check if the change wasn't delete nor merge. Both included checking the operations in the batch. After checking other scenarios I've just used the one from autoformat...

@scofalik
Copy link
Contributor

I understand but still, it's better to be sure. If we can check something for sure with low cost, I'd rather have that than a heuristic, so I've made a follow-up.

@jodator
Copy link
Contributor Author

jodator commented Jul 22, 2019

So I'll fix this with #214 as this is much needed IMO :)

@jodator
Copy link
Contributor Author

jodator commented Jul 23, 2019

@scofalik please re-review this with Input.isInput() added.

*
* **Note:** This method checks if the batch was created using {@link module:typing/inputcommand~InputCommand 'input'}
* command as typing changes coming from user input are inserted to the document using that command.
*
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be nice to provide an example here. Check the issue discussion -- I've posted an example snippet there.

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't cost us much and if anyone would wonder how to exactly use it then they will have a snippet.

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've added a really simple one - probably is enough.

src/inputcommand.js Show resolved Hide resolved
*/
_evaluateTextBeforeSelection( suffix ) {
_evaluateTextBeforeSelection( suffix, data = {} ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need 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.

I'll check.

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 do pass the batch for the matched:data event.

@jodator jodator requested a review from scofalik July 23, 2019 18:01
src/input.js Outdated Show resolved Hide resolved
@scofalik scofalik merged commit 0e26850 into master Jul 24, 2019
@scofalik scofalik deleted the t/208 branch July 24, 2019 08:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants