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 onChangeInitializedEditors callback to CKEditorContext #514

Merged
merged 12 commits into from
Aug 19, 2024
Merged

Conversation

Mati365
Copy link
Member

@Mati365 Mati365 commented Aug 9, 2024

Suggested merge commit message (convention)

Feature: Add an onChangeInitializedEditors callback to CKEditorContext to allow tracking of newly initialized editors within the JSX React tree. Closes #513


Additional information

During the React 19 refactor, it was discovered that CKEditorContext must be initialized before rendering the rest of the React tree (due to StrictMode re-renders on CKEditorContext). This order is also necessary because the rendering of newer React JSX trees is asynchronous, and it is not guaranteed that all CKEditor instances will be registered in the context before onReady is called.

The issue is that some implementations rely on this behavior. Like this one: #513

While we cannot track the full initialization of the entire tree, we can incrementally track what has been added.

⚠️ I'll update docs after approval of this PR

This PR adds:

  1. onChangeInitializedEditors callback to CKEditorContext component, that is fired after any initialization or destruction of any editor in the context.
  2. contextItemMetadata property to CKEditor component that allows onChangeEditorsMap to assign proper name to initialized / or destroyed editor.
Example
<CKEditorContext
  context={ ClassicEditor.Context }
  contextWatchdog={ ClassicEditor.ContextWatchdog }
  onChangeInitializedEditors={ editors => {
    console.log( editors );
  }}
>
  <CKEditor
    editor={ ClassicEditor }
    data="<h2>Editor</h2>"
    contextItemMetadata={{
      name: 'editor1',
      user: { id: '2' }
    }}
  />

  <CKEditor
    editor={ ClassicEditor }
    data="<h2>Another Editor</h2><p>... in common Context</p>"
    contextItemMetadata={{
      name: 'editor2'
    }}
  />
</CKEditorContext>

onWatchInitializedEditors will be called twice in example above:

  1. First log: { editor1: ... }
  2. Second log: { editor1: ..., editor2: ... }

Order of initialization is not guaranteed. editor2 might be initialized before editor1.

@ckeditor ckeditor deleted a comment from coveralls Aug 12, 2024
@Mati365 Mati365 requested a review from illia-stv August 12, 2024 06:50
@Mati365 Mati365 changed the title Add onTrackInitializedEditors callback to CKEditorContext Add onChangeEditorsMap callback to CKEditorContext Aug 12, 2024
Copy link
Member

@filipsobol filipsobol left a comment

Choose a reason for hiding this comment

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

I'm not that good with React, so I will let others do the proper review, but I left some thoughts about the public API.

src/context/useInitializedCKEditorsMap.ts Outdated Show resolved Hide resolved
src/ckeditor.tsx Outdated Show resolved Hide resolved
demos/react/ContextDemo.tsx Outdated Show resolved Hide resolved
@glynam1
Copy link

glynam1 commented Aug 16, 2024

Hi,

Just a question on this implementation so I can confirm closure of #513 when this is merged, onChangeEditorsMap will be fired when an editor is ready right?
So this is equivalent to listening to the onReady callback from an editor.

If this is the same as the callback from
context.editors.on<CollectionChangeEvent<Editor>>( 'change', ( ev, data ) => {

Then I think this won't be useful as the editor is still not actually ready.

@Mati365
Copy link
Member Author

Mati365 commented Aug 16, 2024

@glynam1 It'll be fired when editor is ready (and after destroying of editor). It's the flat map of fully initialized editors that are present in React tree.

@Mati365 Mati365 requested a review from arkflpc August 16, 2024 11:07
@glynam1
Copy link

glynam1 commented Aug 16, 2024

@glynam1 It'll be fired when editor is ready (and after destroying of editor). It's the flat map of fully initialized editors that are present in React tree.

Nice one, looking forward to the update so!

@arkflpc
Copy link

arkflpc commented Aug 16, 2024

LGTM, but the naming is not clear enough.

@Mati365 Mati365 changed the title Add onChangeEditorsMap callback to CKEditorContext Add onWatchInitializedEditors callback to CKEditorContext Aug 19, 2024
@Mati365
Copy link
Member Author

Mati365 commented Aug 19, 2024

@arkflpc I renamed callback to onWatchInitializedEditors. Can you take a look?

Copy link

@gorzelinski gorzelinski left a comment

Choose a reason for hiding this comment

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

I spotted one thing.

demos/react/ContextDemo.tsx Outdated Show resolved Hide resolved
demos/react/ContextDemo.tsx Outdated Show resolved Hide resolved
demos/react/ContextDemo.tsx Outdated Show resolved Hide resolved
@Mati365 Mati365 changed the title Add onWatchInitializedEditors callback to CKEditorContext Add onChangeInitializedEditors callback to CKEditorContext Aug 19, 2024
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 onReady function called before editors attach to context
5 participants