This repository has been archived by the owner on Jun 26, 2020. It is now read-only.
t/225: Made the UI component initialisation and destruction processes synchronous #264
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🎉 |
Reinmar
reviewed
Jul 4, 2017
| ); | ||
| // If `beforeShow` event is not stopped by any external code then panel will be displayed. | ||
| this.once( 'beforeShow', () => { | ||
| if ( isStopped ) { |
There was a problem hiding this comment.
AFAIR, this could've been done using event's cancelling mechanism. Maybe we can change it now?
There was a problem hiding this comment.
Related ticket https://github.com/ckeditor/ckeditor5-ui/issues/232.
Reinmar
reviewed
Jul 4, 2017
src/view.js
Outdated
| * Destroys the view instance and child views located in {@link #_viewCollections}. | ||
| * | ||
| * @returns {Promise} A Promise resolved when the destruction process is finished. | ||
| f * Destroys the view instance and child views located in {@link #_viewCollections}. |
|
I've scanned through the changes in this repo and they are so simple that if the tests pass I'd risk merging without spending more time. |
|
Changes in the other repos look good too. |
|
I'll wait for @oskarwrobel's +1 and merge it. |
oskarwrobel
approved these changes
Jul 4, 2017
oskarwrobel
approved these changes
Jul 4, 2017
| @@ -320,7 +295,6 @@ describe( 'ContextualToolbar', () => { | |||
| beforeEach( () => { | |||
| setData( editor.document, '<paragraph>[bar]</paragraph>' ); | |||
|
|
|||
| // Methods are stubbed because return internal promise which can't be returned in test. | |||
| showPanelSpy = sandbox.stub( contextualToolbar, '_showPanel', () => {} ); | |||
There was a problem hiding this comment.
stub can be changed to spy.
|
Are we OK to merge the constellation? |
|
OK from me. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Suggested merge commit message (convention)
Other: Made the UI component initialization and destruction processes synchronous. Closes ckeditor/ckeditor5#5355.
BREAKING CHANGE:
View#init,View#destroy(alsoViewCollection#init,ViewCollection#destroyandViewCollection#add) no longer returnPromise. It may require updates in UI components which inherit fromViewand rely on the value returned by these methods.BREAKING CHANGE: Various UI components switched to synchronous
init()anddestroy()(no longer returningPromise), which means that features using these components may need some updates to work properly.Additional information
CI branch is https://github.com/ckeditor/ckeditor5/tree/t/ckeditor5-ui/225, also pointing to related branches in other repositories aligning the API to the changes in this issue. They can't be merged one by one so it's all or nothing merge. CI in https://travis-ci.org/ckeditor/ckeditor5/builds/249943542.
Commit message for related branches:
@oskarwrobel: Can you take a look on
ContextualBalloonandContextualToolbarclasses? I made them synchronous but AFAIR because of the asynchronous API the tests were full of hacks. Perhaps you may have some ideas as to how to simplify them even more?