Don't detach pre-existing HTML when view's el is already in the region #3260
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
Resolves marionettejs#3227 This PR also moves some of `Region.show` into a `_setupChildView` function which mirrors what is happening in a CollectionView currently and helps reduce the length of the `show` function which grew to handle the should empty check. In the specs, there was a failing test regarding “when setting the "replaceElement" class option”. I believe this test was failing because it was reusing `this.view` which wasn’t cleaned up in a prior test. So effectively `this.view` was currently already shown in another region. I switched this to `this.view1` which is instantiated as part of this `describe` block (or closer at least) and should be better for these tests.
rafde
approved these changes
Dec 18, 2016
scott-w
reviewed
Dec 18, 2016
| // has been destroyed we can not reuse it. | ||
| view.on('destroy', this._empty, this); | ||
| // Assume an attached view is already in the region for pre-existing DOM | ||
| if (!view._isAttached) { |
There was a problem hiding this comment.
Should we document this assumption?
There was a problem hiding this comment.
I think pre-existing DOM stuff needs its own documentation: #3128 (comment) and this is only a small portion of what isn't fully documented.
scott-w
approved these changes
Dec 23, 2016
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Resolves #3227
This PR also moves some of
Region.showinto a_setupChildViewfunction which mirrors what is happening in a CollectionView currently and helps reduce the length of theshowfunction which grew to handle the should empty check. Additionally moved thecurrentView = viewtoshowto make the attach functions between CollectionView and Region more similar.In the specs, there was a failing test regarding “when setting the "replaceElement" class option”. I believe this test was failing because it was reusing
this.viewwhich wasn’t cleaned up in a prior test. So effectivelythis.viewwas currently already shown in another region. I switched this tothis.view1which is instantiated as part of thisdescribeblock (or closer at least) and should be better for these tests.