AmpersandJS / ampersand-select-view

Select field for ampersand-form-views
MIT License
11 stars 26 forks source link

Doesn't render in el #46

Closed klaemo closed 9 years ago

klaemo commented 9 years ago

Hey, whenever I specify an el the SelectView doesn't get rendered at all. I use it in conjunction with ampersand-form-view. I've tried the latest version on npm and master, all kinds of different combinations of autoAppend and autoRender to no avail.

new SelectView({
  el: this.el.querySelector('[data-hook=package-name]')
})

Without a custom el it works fine. My input-views in the form work fine as well (with custom el). I'm not sure if it's a bug or if I'm doing something wrong, but I have no idea what to try next.

Thank you!

cdaringe commented 9 years ago

Hi @klaemo:

I can't look into this immediately. I wouldn't be surprised if this had something to do with it, but can't say for certain. in the meantime, fiddle?

Thx!

klaemo commented 9 years ago

Thanks for your feedback :) That does look related. It seems as if the parent form view thinks the select-view has already been rendered. I tried removing the check here: https://github.com/AmpersandJS/ampersand-select-view/blob/master/ampersand-select-view.js#L82 but it didn't even get called.

PS: I have fiddled, oh boy have I fiddled!!11 ;-)

klaemo commented 9 years ago

I found the following workaround:

const SelectViewFix = SelectView.extend({
  initialize(opts) {
    SelectView.prototype.initialize.apply(this, arguments)
    this._el = opts._el
  },
  render() {
    SelectView.prototype.render.call(this)
    if (this._el) this._el.appendChild(this.el)
    return this
  }
})

This prevents rendered from being set to true automatically.

cdaringe commented 9 years ago

apologies for late follow up. confirmed, the above issue i suggested is the root of the issue. simply commenting out line 82 does the trick. will try to resolve that issue in effort to close this one

cdaringe commented 9 years ago

@klaemo, I didn't see that you had followed up saying it was unsuccessful for you. I'm not sure how you constructed your example, but I modified the demo/demo.js as simple example. Commenting out the rendered check in this module and using this gist seed as demo.js is all it takes. load 'er up, and you'll see it render into your container el successfully

klaemo commented 9 years ago

Nope your demo doesn't work for me. bildschirmfoto 2015-05-13 um 11 39 58 bildschirmfoto 2015-05-13 um 11 40 16

cdaringe commented 9 years ago

hmm. here is the process i just ran:

it all worked out! can you do a double check? gotta get that rendered issue ironed out

screen shot 2015-05-21 at 6 02 52 pm screen shot 2015-05-21 at 6 03 46 pm screen shot 2015-05-21 at 6 03 50 pm screen shot 2015-05-21 at 6 04 41 pm

klaemo commented 9 years ago

That's so weird! I did pretty much the same thing, but it doesn't work. I even updated to the latest &-view just to be safe.

git status changes to demo

I'm sorry that I can't be of any more help :(

klaemo commented 9 years ago

btw, I just manually applied your fix from https://github.com/AmpersandJS/ampersand-view/pull/119 and I can confirm that it solves this issue as well. :)

wraithgar commented 9 years ago

@klaemo mind submitting a PR for this repo too then?

cdaringe commented 9 years ago

@wraithgar patching this repo probably isn't the ideal solution until a decision is made on the &view 'rendered' bug. Would appreciate some follow up on that one! Link above

klaemo commented 9 years ago

@wraithgar I think here we only need to bump the ampersand-view dep once a version with the fix has been published. Or am I missing something?

cdaringe commented 9 years ago

@klaemo, yea, you are correct.

cdaringe commented 9 years ago

4.3.1 released, with 8.0.0 dep! this should do the trick!