Addepar / ember-widgets

https://opensource.addepar.com/ember-widgets/#/ember-widgets/overview
Other
288 stars 75 forks source link

View registry 1.13 compat #269

Closed mixonic closed 6 years ago

mixonic commented 6 years ago

In Ember 1.13 views instantiated via create().appendTo() do not have a view registry, and thus do not participate in event delegation (a click() { handler on a view/component).

This PR refactors the modal popup( method to use the {{render-popover}} based system for rendering. Included are tests for the new system using the modal base class and handling actions and events.

twokul commented 6 years ago

tests are unhappy

not ok 17 Chrome 68.0 - Integration | Component | render popover: it renders popovers
    ---
        actual: >
            null
        stack: >
            Error: Assertion Failed: You have turned on testing mode, which disabled the run-loop's autorun.
                              You will need to wrap any code with asynchronous side-effects in a run
                at new EmberError (http://localhost:7357/assets/vendor.js:30897:21)
                at Object._emberMetalCore.default.assert (http://localhost:7357/assets/vendor.js:19616:13)
                at checkAutoRun (http://localhost:7357/assets/vendor.js:35827:34)
                at Function.run.scheduleOnce (http://localhost:7357/assets/vendor.js:35583:5)
                at Class.scheduleRevalidate (http://localhost:7357/assets/vendor.js:64909:40)
                at http://localhost:7357/assets/vendor.js:27023:32
                at Stream.notifySubscribers (http://localhost:7357/assets/vendor.js:36402:11)
                at Stream.notifyExcept (http://localhost:7357/assets/vendor.js:36342:14)
                at Stream.notify (http://localhost:7357/assets/vendor.js:36336:12)
                at KeyStream.notifySubscribers (http://localhost:7357/assets/vendor.js:36404:20)
        message: >
            Died on test #3     at Module.callback (http://localhost:7357/assets/tests.js:544:24)
                at Module.exports (http://localhost:7357/assets/vendor.js:111:32)
                at requireModule (http://localhost:7357/assets/vendor.js:32:18)
                at TestLoader.<anonymous> (http://localhost:7357/assets/test-support.js:13973:11)
                at TestLoader.require (http://localhost:7357/assets/test-support.js:13963:27)
                at TestLoader.loadModules (http://localhost:7357/assets/test-support.js:13955:16)
                at loadTests (http://localhost:7357/assets/test-support.js:14706:22): Assertion Failed: You have turned on testing mode, which disabled the run-loop's autorun.
                              You will need to wrap any code with asynchronous side-effects in a run
mixonic commented 6 years ago

@twokul @bantic @kybishop This is ready for review. As I looked into having our downstream usage adapt to this API change (modal popups will need to be passed the container) I also noted that some usage expected the ability to close the modal based on a return value from popup. I've added a return value from both the popover and modal that will close the opened item, and also tested that existing ways of closing the items via event dispatching still work.

bantic commented 6 years ago

lgtm. Caught a small typo in tests, but this looks good.