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

Use async/await in CKEditorContext#_destroyContext() to fix the destruction process #284

Merged
merged 2 commits into from
Feb 9, 2022

Conversation

Mgsy
Copy link
Member

@Mgsy Mgsy commented Feb 8, 2022

Suggested merge commit message (convention)

Fix: Use async/await in CKEditorContext#_destroyContext() to handle context destruction properly. Closes #283.


It is not that easy to test, so here is a short guide explaining how to do it:

  1. Get the real-time-collaboration-comments-outside-of-editor-for-react collaboration sample and replace original src/sample.js with my code.
  2. In src/sample.js add import path to the local ckeditor5-react package.
  3. Build and run the sample.
  4. Open the dev tools and the network tab.
  5. Click the button to create editors.
  6. The WebSocket connection should be pending.
  7. Click the button to destroy editors.
  8. The WebSocket connection should be closed.

The better description and GIFs are available in the original issue.

@Mgsy Mgsy requested a review from pomek February 8, 2022 13:55
@coveralls
Copy link

coveralls commented Feb 8, 2022

Pull Request Test Coverage Report for Build 376

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 366: 0.0%
Covered Lines: 152
Relevant Lines: 152

💛 - Coveralls

@pomek pomek merged commit af2cdba into master Feb 9, 2022
@pomek pomek deleted the i/283 branch February 9, 2022 10:21
Copy link
Member

@pomek pomek left a comment

Choose a reason for hiding this comment

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

Post-merge LGTM.

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.

CKEditorContext component doesn't destroy properly
3 participants