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.

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

Make sure $el contains only the el #3426

Merged
merged 1 commit into from
Aug 17, 2017

Conversation

paulfalgout
Copy link
Member

@paulfalgout paulfalgout commented Aug 14, 2017

Alternative solution to #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 added this to the v3.4.2 milestone Aug 14, 2017
@paulfalgout paulfalgout requested a review from JSteunou August 14, 2017 07:30
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 0fa0480 on paulfalgout:bugfix/region-select-one into 1962fbb on marionettejs:next.

@paulfalgout paulfalgout changed the title WIP make sure $el contains on the el WIP make sure $el contains only the el Aug 14, 2017
@JSteunou
Copy link
Contributor

Calling getEl twice eh... Kinda ironic. Wouldnt be better if the 1st call always return only one element. I dont see where we use multiple elements from getEl

@paulfalgout
Copy link
Member Author

I can't find a way in which returning a single element from region.getEl isn't far more complicated.
FWIW all I did was move a getEl from attach which is called on every show up into the show which will only be called the first time. We could maybe just call this.Dom.getEl directly.. That might be better here.

@paulfalgout paulfalgout force-pushed the bugfix/region-select-one branch from 0fa0480 to f930ef0 Compare August 15, 2017 05:34
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling f930ef0 on paulfalgout:bugfix/region-select-one into 1962fbb on marionettejs:next.

@scott-w
Copy link
Member

scott-w commented Aug 15, 2017

Would jQuery .first() not work to resolve that issue? https://api.jquery.com/first/

@paulfalgout
Copy link
Member Author

We shouldn't require jQuery here at all.

@scott-w
Copy link
Member

scott-w commented Aug 15, 2017

I meant inside the DOM API somewhere.

@paulfalgout
Copy link
Member Author

I would consider that a breaking change

@paulfalgout
Copy link
Member Author

I suppose it is still an experimental API. still though.. I'm not sure what the concern is about this change. it is 100% the same code that is currently ready for v3.4.2 it's just that the Dom.getEl is now called once for a region instead of every show.

@scott-w
Copy link
Member

scott-w commented Aug 15, 2017

I'm fine with it I just wanted to play devil's advocate 😈

@JSteunou
Copy link
Contributor

Same... The solution just seems clunky even if right, I cannot stop thinking it must be a more elegant way

@paulfalgout
Copy link
Member Author

I think the more elegant way comes with #3417 and removing any sort of getEl from regions altogether.

@JSteunou
Copy link
Contributor

What about here ? We could return the 1st element directly.

@JSteunou
Copy link
Contributor

Which conflict with #3425

Ok no more ideas :D

👍

@scott-w
Copy link
Member

scott-w commented Aug 16, 2017

I'm happy with this, we should add a test, however.

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
@paulfalgout paulfalgout changed the title WIP make sure $el contains only the el Make sure $el contains only the el Aug 17, 2017
@paulfalgout paulfalgout force-pushed the bugfix/region-select-one branch from f930ef0 to 1e5fe04 Compare August 17, 2017 06:43
@paulfalgout
Copy link
Member Author

updated with a test

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 1e5fe04 on paulfalgout:bugfix/region-select-one into 4af7db8 on marionettejs:next.

@JSteunou JSteunou merged commit 575bf47 into marionettejs:next Aug 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants