search

kybishop / ember-popper

Popper.js for Ember
MIT License
39 stars 35 forks source link

fix: use @ember/render-modifiers instead of ember-ref-modifier to avoid timing issues #127

Closed richard-viney closed 4 years ago

richard-viney commented 4 years ago

The way the {{ref}} modifier defers its sets causes unpredictable timing issues in popovers.

Using the simpler {{did-insert}} and {{will-destroy}} modifiers is more robust and fixes the timing issues I've encountered. I expect it also fixes kybishop/ember-popper#126.

Doing it this way also avoids needing to hook into the set() on _popperElement, because with {{did-insert}} we get notified directly when the element is inserted.

Thoughts?

richard-viney commented 4 years ago

I suspect that https://github.com/lifeart/ember-ref-modifier/pull/232 would also fix this problem, were it to be merged.

However, even if that's the case I still think that ember-popper should use the simpler {{did-insert}} and {{will-destroy}} modifiers that more directly provide the required functionality here.

richard-viney commented 4 years ago

@kybishop Wondering if you'd had a chance to look at this? I've been using it for a while now with no issues.

kybishop commented 4 years ago

Hey @richard-viney, I'm currently at a full-time gig that exclusively uses React, which leaves me mostly hands-off on Ember these days. @simonihmig or @tylerturdenpants would probably be better points of contact.

richard-viney commented 4 years ago

I've removed the didRender() hook. It was likely resulting in a double-call to updatePopper(), but not in a way that really mattered thanks to the change detection in that function.

didRender() was also being used to update the popper when other arguments changed, e.g. placement. I've replaced this with the {{did-update}} modifier. Having to repeat the {{did-update}} code 4 times isn't ideal, but I don't want to add an extra div just for {{did-update}}, and anything else would require somewhat larger changes I don't want to include in this PR.

Incidentally, a full Octane upgrade for this addon would be nice. This PR now takes a few initial steps in that direction by dropping some lifecycle hooks etc.

simonihmig commented 4 years ago

Just published this as v0.11.3, thanks for your work here again @richard-viney!