throw error when region has more than one element #3341
Closed
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
Rebuild of CollectionView and children container
Along the lines of `reorder` -> `sort` and `viewComparator`, this change names it what it is and makes them more similar. It also, and most importantly answers the question, “How do I reapply a filter” which has been a reoccurring question on gitter.
This does two things. It is more explict what it means when called. It allows for `_removeChildView` to be called within an `_.each` when the 2nd argument is the index.
after pulling master into next the linter failer for these imports
Backbone does it
`myCollection.filter({ foo: true });`
`myCollection.filter(‘foo’);`
We might as well:
`myCollectionView.setFilter({ foo: true });`
`myCollectionView.setFilter(‘foo’);`
Also `filter` needs to return `this`.
Really we should bail out quick if not viewfilter is set and throw an error if the filter won’t work. Also `filter` shouldn’t occur if there are no children. In the normal `render` work flow the `emptyView` would show prior to `filter` and `filter` would never be called.. so this handles when a user calls `filter` and there are no children (possibly because the view has not yet been rendered) Because `filter` handles no children, `setFilter` does not need to care about `children` or if the view `isRendered`.
Adds: - `getViewFilter` similar to `getViewComparator` - `setComparator` similar to `setFilter` - `removeComparator` similar to `removeFilter`
Until marionettejs#3247 is in, the `_parent` here will skip over the `CollectionView` and trigger on any `View` above the `CollectionView`. For that reason, currently setting the `_parent` here is a bad idea and users will have to listen to the `emptyRegion` or `emptyRegion.currentView`. Until a CollectionView can be treated as a parent of a Region we should leave this line out 😿 Additionally the emptyRegion should be destroyed on clean up
Implements merged feature
Implements merged feature
If there’s nothing to sort, no need to continue. This matches filter functionality.
Underscore uses native bind if available, but lodash does not. This only worked with native bind, so we should continue that. IE9+ here we come. (or polyfill function.prototype.bind)
recreating `children` would break anyone's reference to the `children` object before a `render`. This prevents that issue.
This fixes removing a view that isn't in the `children` causing the `children` to be spliced oddly. Additionally it reuses `removeChildView` in `detachChildView`
Views will be an array of views that were rendered and not filtered out. Resolves marionettejs#2570
We're using `setFilter` and `removeFilter`.. `getFilter` only makes sense
…lection-view New CollectionView
This also makes `_detachView` more similar to `_attachView` - [x] Test fails without code changes
Resolves: marionettejs#1579 The use-case here is that often times you want to initialize a behavior with things relative to the initialized view, but the behavior itself is initialized prior to the view's initialization so that it's events can be delegated. This new event allows for developers to access view properties once the view is ready ```js var MyBehavior = Marionette.Behavior.extend({ onInitializeView: function(view) { view.$el.addClass('my-behavior'); } }); ``` Todo: - [ ] Tests - [ ] Docs - [ ] Apply this to marionettejs#3244 upon merge of one or the other
The test following this one is identical except that it is more thorough.
|
@marionettejs/marionette-core feeling on whether or not this should be considered breaking? |
|
When writing this, I figured it would be considered breaking. There may be cases where multiple elements are found for a region and the app functions perfectly fine (happen to be the first element in the list). |
|
@paulfalgout It's definitely a major and breaking change. This is best something for Mn 4. |
|
I agree, this is a breaking change. While a developer shouldn't add regions where selectors will hit multiple nodes, the fact is that they can and, thus, probably have. I'd happy 👍 this for Mn 4 as I think it's the right way to go. |
paulfalgout
force-pushed
the
next
branch
2 times, most recently
from
328bb91 to
dd36023
Compare
|
@erikahlswede could you rebase your branch ? |
|
FWIW I'm no longer certain this should go in v4. I think perhaps Region has some problems that could be resolved better than an error. |
|
I think in the end the solution will be handled differently. Too many caveats in the error. |
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.
Proposed changes
Link to the issue:
#3320
I played around with a warning mechanism...but at the end of the day, it was☹️
console.warn. I like the idea of throwing an error in this situation. This way, it is much more clear to the user, rather than having views show up in the wrong spots