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

T/4: Add debounce mechanism. #65

Merged
merged 56 commits into from
Dec 18, 2019
Merged

T/4: Add debounce mechanism. #65

merged 56 commits into from
Dec 18, 2019

Conversation

jodator
Copy link
Contributor

@jodator jodator commented May 9, 2019

Suggested merge commit message (convention)

Type: Add debounce mechanism for requesting a feed. Closes ckeditor/ckeditor5#4619.


Additional information

  • This PR solves the problem of asynchronous server by:
    • Adding a debounce mechanism for requesting a feed (relatively short - 100ms so if the feed callback promise resolves within 100-150ms it should still be perceived as "fast". - note that toolbar plugin is debounced to 200ms.
    • Handling errors and request out-of-order problems by:
      • discarding all but last requested feed (the comparison i relatively simple - just remember the last requested feed (the text for it) and if the response is not for this feed discard it
      • showing warning notification for any errors that ocured in feed callback (most likely due to communication/connectivity problems)

@jodator jodator marked this pull request as ready for review May 9, 2019 12:28
@jodator jodator requested review from mlewand and Mgsy May 9, 2019 12:29
@jodator
Copy link
Contributor Author

jodator commented May 9, 2019

@Mgsy As this is issue should be prone to manual tests please take a look at the new manual tests for asynchronous feeds in mentions (others should work too).

@mlewand What is done is in the description but I've added a small follow up: #50 that can be resolved after this PR.

@jodator
Copy link
Contributor Author

jodator commented May 9, 2019

Edge.... :(

@jodator
Copy link
Contributor Author

jodator commented May 9, 2019

🤞 last fixes to the test were pushed - hopefully will be OK now.

@jodator
Copy link
Contributor Author

jodator commented Jul 25, 2019

@Reinmar thanks for the insights. I've cleaned up the code a bit but still, some minor work is needed:

  1. Add tests for updating a marker.
  2. Check some ifs if they cannot have some better code (e.g. adding isInCompletionMode() check).
  3. Document the events.

But if feel free to check the code again if you wish - I don't think that much will change :)

@jodator
Copy link
Contributor Author

jodator commented Jul 26, 2019

@Mgsy Could you please re-check this PR as before? This should work the same - ie no errors, etc. The code was refactored but it looks like everything works as expected.

@Reinmar So it is ready to review. The only thing that I've wanted to ask is should we expose events? I'm not sure if they are important to others. Right now they are documented but without much fuzz around them.

# Conflicts:
#	package.json
@jodator
Copy link
Contributor Author

jodator commented Dec 16, 2019

@mlewand the PR is OK now

@mlewand mlewand assigned mlewand and unassigned Reinmar Dec 17, 2019
Copy link
Contributor

@mlewand mlewand left a comment

Choose a reason for hiding this comment

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

LGTM pushed couple minor details.

Note: 🙈 there might be 6 outdated comments, from months ago back when I was doing previous review. GH shows the count in rev summary window, but doesn't show the comments in the "Files changed" view so I can't remove them 🤔

image

@mlewand mlewand merged commit f50db9c into master Dec 18, 2019
@mlewand mlewand deleted the t/4 branch December 18, 2019 09:21
@FilipTokarski
Copy link
Member

Tested on all browsers, seems to work fine now 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add debounce mechanism to opening the mention panel
5 participants