search

ckeditor / ckeditor4-vue

Official CKEditor 4 Vue component
Other
75 stars 17 forks source link

Use delayIfDetached as default value in editor config #128

Closed sculpt0r closed 2 years ago

sculpt0r commented 2 years ago

Bump vue testing utility. It is no longer in the beta version. The newest version contains options for mount testing method which is useful in this case: https://vue-test-utils.vuejs.org/api/options.html#attachto

Adjust the component to use the new default config value. It requires moving editor inner instance initialization to the instanceReady event. Since with the delayIfDetach config options, it is possible that core replace or inline methods return null instead of editor instance.

Split mounted component logic onto separate methods - since preparing config for the editor is more complicated now.

Add safari launcher since BS Safari often ends with timeout errors. So it is possible to use safari in karma config on your local machine. However, I didn't modify the config.

As for the tests workflow. Hopefully, I was able to separate tests and test cases from each other. I have to make an ugly hack for the test with an empty editor config since I can't set there a fake observableParent. MutationObserver is disconnected while the editor switch mode, so I used that. But because of those problems, I made a follow up:

Currently, we are destroying the wrapper for the CKE4, however, we may destroy the editor first, and then clean up the wrapper. Currently, it makes no difference, but I've made a follow up: https://github.com/ckeditor/ckeditor4/issues/5004

Extracted two functions in tests to util file.

Also, test changes may be a good introduction to https://github.com/ckeditor/ckeditor4-vue/issues/6 However - I feel like we need to put more effort to test refactoring.

Closes #102 , #86

github-actions[bot] commented 2 years ago

It's been a while since we last heard from you. We are marking this pull request as stale due to inactivity. Please provide the requested feedback or the pull request will be closed after next 7 days.

sculpt0r commented 2 years ago

Rebased on the newest master.

sculpt0r commented 2 years ago

I've also seen that a safari launcher is added but it's not used at all. The karma config is not modified 🤔. Shouldn't it be added in place of BS Safari? Or at least added to the list of local browsers? However, it'd probably require making browsers list os-dependent.

I didn't touch the karma config. As you mentioned that will require additional changes around it. I thought it may be a nice option for the developer to just change the config without searching and installing the launcher. Do you want me to remove it from dependencies since it is not explicitly used in the code - @Comandeer ?

Comandeer commented 2 years ago

Do you want me to remove it from dependencies since it is not explicitly used in the code - @Comandeer ?

Yes, it could be extracted as a separate ticket.

sculpt0r commented 2 years ago

@Comandeer - ready for another review.