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 article describing the delayed editor initialisation feature #363

Closed
wants to merge 15 commits into from

Conversation

sculpt0r
Copy link
Contributor

Added a short article explaining in practice, what is the difference between two options in delayed editor creation.

Closes ckeditor/ckeditor4#4553

@f1ames f1ames self-requested a review July 26, 2021 15:12
@f1ames f1ames self-assigned this Jul 26, 2021
Copy link
Contributor

@f1ames f1ames left a comment

Choose a reason for hiding this comment

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

I like the idea of the separate section and the broad description since the concept described here is not that easy to understand 👍 Still, I would try to focus on making the description as simple as possible (which doesn't mean short) to make it easy to understand. I added few suggestions and corrections, but feel free to adjust them further.

I also wonder if we should use term like "editor (parent) element" or maybe "editor container"?

Hmmm, we have both Guides and Features section which is sometimes confusing. Guides are kind of dev guides so the topic of delaying editor (as this is configuration stuff and may require coding) fits there. OTOH it's a feature which can be simply enabled via config so fits in Features section too 🤔
I guess it will be good to have discoverability in mind (how the person not knowing about this feature can find it).

Apart from that I see common cases where it is useful are already mentioned but it will be good to mention cases where it might break something (e.g. editor not initializing) so one may try to disable this feature to solve the issue.

docs/features/delayed_creation/README.md Outdated Show resolved Hide resolved
docs/features/delayed_creation/README.md Outdated Show resolved Hide resolved
docs/features/delayed_creation/README.md Outdated Show resolved Hide resolved
docs/features/delayed_creation/README.md Outdated Show resolved Hide resolved
docs/features/delayed_creation/README.md Outdated Show resolved Hide resolved
docs/features/delayed_creation/README.md Outdated Show resolved Hide resolved
docs/features/delayed_creation/README.md Outdated Show resolved Hide resolved
@sculpt0r sculpt0r self-assigned this Jul 28, 2021
@sculpt0r
Copy link
Contributor Author

sculpt0r commented Jul 29, 2021

I also wonder if we should use term like "editor (parent) element" or maybe "editor container"?

I thought about that. Probably I should check if I'm 100% consistent here, but:

  • I prefer "editor parent element" when we talk about detaching elements. Because not only the closest parent may be detached, but any parent element. And by "editor container" (even according to our API) we are calling the closest parent. 🤔

Hmmm, we have both Guides and Features section which is sometimes confusing. Guides are kind of dev guides so the topic of delaying editor (as this is configuration stuff and may require coding) fits there. OTOH it's a feature which can be simply enabled via config so fits in Features section too 🤔
I guess it will be good to have discoverability in mind (how the person not knowing about this feature can find it).

My way of thinking: Guides are for people who just start with CKEditor or try to achieve some functionality. Features are like "more advanced things" which you can use if you are more proficient with the editor. Also, it was made as a 'feature' during development and it is a kind of non-standard usage.

@sculpt0r
Copy link
Contributor Author

Hi @f1ames, ready for another review. The changes:

  • I correct some ;p;p of the paragraphs
  • Added a troubleshooting paragraph - which should exactly show what was the problem with the detached element. And finally, I resign from linking to any GH issue.
  • I separate almost every paragraph on two sections: the interval and the callback - which should better visualize differences between those two approaches.
  • Added a paragraph how to grab the editor reference when this feature is usage.

@sculpt0r sculpt0r requested a review from f1ames July 29, 2021 11:09
@f1ames f1ames self-assigned this Jul 30, 2021
Copy link
Contributor

@f1ames f1ames left a comment

Choose a reason for hiding this comment

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

Looks good, I did some corrections here and there - could you take a look?

I still have a feeling that we are missing basic, general overview what this feature does. It is easy to understand for us, but when reading docs it seems we go right away into how to use it and approaches there are. But the introduction is a bit laconic 🤔 WDYT? Maybe we could expand it somehow?

Comment on lines +113 to +116
### Interval approach
```js
var editor = CKEDITOR.instances[ 'editorName' ];
```
Copy link
Contributor

Choose a reason for hiding this comment

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

I would mention here that getting editor ref should be done on instanceReady event. Because when this code is called before editor is created it will fail AFAIR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, you can grab an instance like that, but need to wait for instanceReady anyway to use API. Here, I just want to point out that the old way of creating an editor may return null whereas the object might be returned in the old code.

But, yes - I will mention that :)

@sculpt0r sculpt0r self-assigned this Aug 3, 2021
@f1ames
Copy link
Contributor

f1ames commented Aug 3, 2021

One more thing I forgot to mention during review 🙈 This PR relates to changes in the upcoming CKEditor 4 major so should target major branch.

@sculpt0r sculpt0r changed the base branch from stable to major August 3, 2021 12:25
@sculpt0r sculpt0r changed the base branch from major to stable August 3, 2021 12:27
@sculpt0r
Copy link
Contributor Author

sculpt0r commented Aug 3, 2021

One more thing I forgot to mention during review 🙈 This PR relates to changes in the upcoming CKEditor 4 major so should target major branch.

The rebasing was kind of painful... So I decided to create another PR but saving commits from this one: #364. Sorry about that.

@sculpt0r sculpt0r closed this Aug 3, 2021
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.

Add article describing the delayed editor initialisation feature
4 participants