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

I/6020: Run only one instance of the TextWatcher for all text transformations #223

Merged
merged 20 commits into from
Mar 10, 2020
Merged

I/6020: Run only one instance of the TextWatcher for all text transformations #223

merged 20 commits into from
Mar 10, 2020

Conversation

panr
Copy link
Contributor

@panr panr commented Feb 3, 2020 •
edited by Reinmar
Loading

Suggested merge commit message (convention)

Other: Run only one instance of the TextWatcher for all text transformations. Closes ckeditor/ckeditor5#6020.

@panr
Tests: Update texttransformation manual test config and docs
3772c7a
@panr
Fix: Reduce the TextWatcher instances creation to only one
7bd91b9 
This commit also changes the TextTransformation's API adding:
- #configuredTransformations
- #_normalizedConfiguredTransformations
- #_setNormalizedTransformations()
@panr
Other: Get transformation from the match if it occurs and pass it to …
d701283 
…the event data object
@panr panr changed the title I/6020: Run only one instance of the TextWatcher for alle text transformations I/6020: Run only one instance of the TextWatcher for all text transformations Feb 3, 2020
@coveralls
Copy link

coveralls commented Feb 3, 2020 •
edited
Loading

Coverage Status

Coverage remained the same at 100.0% when pulling b713427 on i/6020 into 64daf31 on master.

@panr
Copy link
Contributor Author

panr commented Feb 3, 2020 •
edited
Loading

Do we need some tests to cover the changes or these are redundant? 🤔
Just added.

panr
panr commented Feb 3, 2020
View reviewed changes
src/texttransformation.js Outdated Show resolved Hide resolved
panr
panr commented Feb 3, 2020
View reviewed changes
src/texttransformation.js Outdated Show resolved Hide resolved
@panr panr requested a review from Reinmar February 25, 2020 08:44
@Reinmar
Copy link
Member

Reinmar commented Feb 27, 2020

MINOR BREAKING CHANGES: This PR changes the TextTransformation's API, adding:

Adding API is not breaking anything. Those are new things which no one used so far, so it couldn't break anything.

Reinmar
Reinmar reviewed Feb 27, 2020
View reviewed changes
src/texttransformation.js Outdated Show resolved Hide resolved
Reinmar
Reinmar reviewed Feb 27, 2020
View reviewed changes
tests/manual/texttransformation.md Outdated
* Change selection - the not transformed elements should stay.
* Change selection - the not transformed elements should stay.

### Behaviour inside the code blocks
Copy link
Member

Choose a reason for hiding this comment

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

Where do those changes come from? The ticket is about not creating multuple instances of text watcher. I thought we resolved the code block issue previously.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I should have added this before... I will update the test in another PR (or maybe this can be added on master directly?).

Reinmar
Reinmar suggested changes Feb 27, 2020
View reviewed changes
Copy link
Member

@Reinmar Reinmar left a comment

Choose a reason for hiding this comment

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

As commented. Better to keep those changes local and minimal.

@panr panr requested a review from Reinmar February 27, 2020 21:05
@panr
Copy link
Contributor Author

panr commented Feb 27, 2020

As commented. Better to keep those changes local and minimal.

Done 👍

Reinmar
Reinmar reviewed Mar 2, 2020
View reviewed changes
src/textwatcher.js Outdated Show resolved Hide resolved
Reinmar
Reinmar suggested changes Mar 2, 2020
View reviewed changes
Copy link
Member

@Reinmar Reinmar left a comment

Choose a reason for hiding this comment

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

I found one issue (my initial proposal was a bit incorrect). Plus, I realised that there are no tests here. How do we know that the issue was resolved and prevent from it happening in the future?

@Reinmar
Copy link
Member

Reinmar commented Mar 2, 2020

I found one issue (my initial proposal was a bit incorrect). Plus, I realised that there are no tests here. How do we know that the issue was resolved and prevent from it happening in the future?

We also miss tests for the new TextWatcher functionality.

@panr panr requested a review from Reinmar March 2, 2020 14:31
@panr
Copy link
Contributor Author

panr commented Mar 2, 2020 •
edited
Loading

Ready for re-review 🙈

@panr
Copy link
Contributor Author

panr commented Mar 3, 2020

22e6de7

I added ref docs for {module:typing/textwatcher~TextWatcher#testCallback}. Let me know if this is OK.

@Reinmar
Copy link
Member

Reinmar commented Mar 4, 2020 •
edited
Loading

TODO:

  • bring back tests for boolean testCallback results and verify that there are no breaking changes in this PR (e.g. that the mention feature works fine),
  • validate the API docs,
  • review docs for `matched:data` once again; mention the additional data there,
  • we also miss tests for the new TextWatcher functionality.

Reinmar
Reinmar reviewed Mar 4, 2020
View reviewed changes
tests/textwatcher.js Outdated Show resolved Hide resolved
@panr panr requested a review from Reinmar March 4, 2020 13:20
@panr
Copy link
Contributor Author

panr commented Mar 6, 2020

Ready for re-review.

@Reinmar Reinmar merged commit 550426d into master Mar 10, 2020
@Reinmar Reinmar deleted the i/6020 branch March 10, 2020 20:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Reinmar Reinmar Awaiting requested review from Reinmar

Assignees

@Reinmar Reinmar

Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@panr @coveralls @Reinmar