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

Add support for the Context and Watchdog features #178

Merged
merged 40 commits into from
Sep 18, 2020
Merged

Add support for the Context and Watchdog features #178

merged 40 commits into from
Sep 18, 2020

Conversation

ma2ciek
Copy link
Contributor

@ma2ciek ma2ciek commented Sep 8, 2020

Suggested merge commit message (convention)

Feature: The <CKEditor> component contains the built-in watchdog feature. Closes #118.

Feature: Introduced the <CKEditorContext> component that supports the context feature.

Feature: Added the id property which is used to distinguish different documents. When this property changes, the component restarts the underlying editor instead of setting data on it, which allows e.g. for switching between collaboration documents and fixes a couple of issues (e.g. the onChange event no longer fires during changing the document). Closes #168. Closes #169.

Feature: The onError() callback will be called with two arguments. The first one will be an error object (as it was before the release 3+). A second argument is an object that contains two properties:

  • {String} phase: 'initialization'|'runtime' - Informs when the error has occurred (during the editor/context initialization or after the initialization).

  • {Boolean} willEditorRestart - When true, it means that the editor component will restart itself.

  • {Boolean} willContextRestart- When true, it means that the context component will restart itself.

    The willEditorRestart property will not appear when the error has occurred in the context feature.
    The willContextRestart property will not appear when the error has occurred in the editor.


  • Add to the release summary: Both components (<CKEditor> and <CKEditorContext>) will internally use the Watchdog class that restarts the editor or context when an error occurs.

  • Add to the release summary: The API of the entry point of the package has changed.

The previous way how the package was imported:

import CKEditor from '@ckeditor/ckeditor5-react';

will not work with the release (3+). Use:

import { CKEditor } from '@ckeditor/ckeditor5-react';

BREAKING CHANGE: The entry point of the package has changed. The default import was removed since the package provides more than a single component now. Use import { CKEditor } from '@ckeditor/ckeditor5-react' instead of import CKEditor from '@ckeditor/ckeditor5-react';.

BREAKING CHANGE: The onInit property was renamed to onReady and can be called multiple times (after the initialization and after the component is ready when an error occurred).


Additional information

[TODO]: Add a link to PR in ckeditor5.

@coveralls
Copy link

coveralls commented Sep 8, 2020

Pull Request Test Coverage Report for Build 309

  • 61 of 68 (89.71%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-9.2%) to 90.789%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/ckeditorcontext.jsx 26 29 89.66%
src/ckeditor.jsx 35 39 89.74%
Totals Coverage Status
Change from base Build 231: -9.2%
Covered Lines: 95
Relevant Lines: 102

💛 - Coveralls

@ma2ciek
Copy link
Contributor Author

ma2ciek commented Sep 9, 2020

I see there're some uncovered lines touching the error handling mechanisms. I'll cover them later today.

@ma2ciek ma2ciek requested a review from pomek September 9, 2020 08:20
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.

First-round. Mostly I am missing tests.

README.md Outdated Show resolved Hide resolved
src/ckeditor.jsx Outdated Show resolved Hide resolved
src/context.jsx Outdated Show resolved Hide resolved
src/context.jsx Outdated Show resolved Hide resolved
src/utils/uid.js Outdated Show resolved Hide resolved
tests/context.jsx Outdated Show resolved Hide resolved
tests/context.jsx Outdated Show resolved Hide resolved
tests/context.jsx Outdated Show resolved Hide resolved
tests/ckeditor.jsx Outdated Show resolved Hide resolved
tests/ckeditor.jsx Outdated Show resolved Hide resolved
src/ckeditor.jsx Outdated Show resolved Hide resolved
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.

We need to decide about the AIP in the onReady/onError callbacks.

Also, minor refactoring would be nice.

Missing tests (and dropped coverage) could be fixed in a separate issue.

README.md Outdated Show resolved Hide resolved
samples/index.html Outdated Show resolved Hide resolved
src/ckeditor.jsx Outdated Show resolved Hide resolved
src/ckeditor.jsx Outdated Show resolved Hide resolved
src/ckeditor.jsx Outdated Show resolved Hide resolved
src/ckeditor.jsx Outdated Show resolved Hide resolved
src/context.jsx Outdated Show resolved Hide resolved
src/ckeditor.jsx Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
…watchdog in the editor component. Changed API for `onReady()` and onError() functions.
README.md Outdated Show resolved Hide resolved
src/ckeditor.jsx Outdated Show resolved Hide resolved
src/ckeditor.jsx Outdated Show resolved Hide resolved
@pomek pomek mentioned this pull request Sep 18, 2020
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.

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
3 participants