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.

Sign up for GitHub

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

Jump to bottom

Region appendView use $el instead of el #3421

Closed
JSteunou opened this issue Aug 9, 2017 · 0 comments · Fixed by #3422
Closed

Region appendView use $el instead of el #3421

JSteunou opened this issue Aug 9, 2017 · 0 comments · Fixed by #3422
Assignees
@JSteunou
Labels
bug
Milestone
v3.4.2

Comments

@JSteunou
Copy link
Contributor

JSteunou commented Aug 9, 2017 •
edited
Loading

Tricky regression from v3.4

In some context, a View could have a region with a selector like [data-region="body"] and another region with a childview with also [data-region="body"] in its own DOM

Before v3.4 the append logic casted the region.el into jquery object to put the view content in it. The region.el is always the region.$el 1st element. See here & here

Since v3.4 the append logic uses the region.$el that could contains more than one element, thus the append will put the view inside the last element. See here & here
JSteunou added a commit that referenced this issue Aug 9, 2017
@JSteunou
Do not use region $el, prefer el refs #3421
1c6760b
@JSteunou JSteunou mentioned this issue Aug 9, 2017
Merged
@JSteunou JSteunou self-assigned this Aug 9, 2017
@JSteunou JSteunou added the bug label Aug 9, 2017
@JSteunou JSteunou added this to the v3.4.2 milestone Aug 9, 2017
rafde pushed a commit that referenced this issue Aug 9, 2017
@JSteunou @rafde
Do not use region $el, prefer el refs #3421
0f8c543
paulfalgout added a commit to paulfalgout/backbone.marionette that referenced this issue Aug 14, 2017
@paulfalgout
WIP make sure $el contains on the el
0fa0480 
Alternative solution to marionettejs#3421

This is a WIP and needs tests, but I think doing this check once on `_ensureElement` is better than within `attachHtml`  And it makes `this.$el` more consistent and matches what happens in the region constructor

This still needs a test
@paulfalgout paulfalgout mentioned this issue Aug 14, 2017
Merged
paulfalgout added a commit to paulfalgout/backbone.marionette that referenced this issue Aug 15, 2017
@paulfalgout
WIP make sure $el contains on the el
f930ef0 
Alternative solution to marionettejs#3421

This is a WIP and needs tests, but I think doing this check once on `_ensureElement` is better than within `attachHtml`  And it makes `this.$el` more consistent and matches what happens in the region constructor

This still needs a test
paulfalgout added a commit to paulfalgout/backbone.marionette that referenced this issue Aug 17, 2017
@paulfalgout
Make sure $el contains on the el
1e5fe04 
Alternative solution to marionettejs#3421

I think doing this check once on `_ensureElement` is better than within `attachHtml`  And it makes `this.$el` more consistent and matches what happens in the region constructor
JSteunou pushed a commit that referenced this issue Aug 17, 2017
@paulfalgout @JSteunou
Make sure $el contains on the el
575bf47 
Alternative solution to #3421

I think doing this check once on `_ensureElement` is better than within `attachHtml`  And it makes `this.$el` more consistent and matches what happens in the region constructor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@JSteunou JSteunou

Labels
bug
Projects
None yet
Milestone
v3.4.2
Development

Successfully merging a pull request may close this issue.

Do not use region $el, prefer el refs #3421 marionettejs/backbone.marionette
1 participant
@JSteunou