search

Addepar / ember-widgets

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

Add hideOthers option to popover #238

Closed mgreenbaum closed 8 years ago

mgreenbaum commented 8 years ago

Allow users to disable the hideAll that's triggered when launching a popover. This change is necessary for keeping multiple popovers open at the same time.

@billy-addepar

billy-addepar commented 8 years ago

@jordansmith42 @cyril-sf @thangdinh

thangdinh commented 8 years ago

Please write test for this

mgreenbaum commented 8 years ago

from https://github.com/Addepar/ember-widgets/blob/master/CONTRIBUTING.md:

For now, pull requests aren't required to have tests. Once we've established a testing infrastructure, pull requests that introduce new functionality should come with tests for that functionality.

thangdinh commented 8 years ago

It is in 2014, we just didn't update that doc file I think. If you see the recent commits, all have tests. And the statement in that doc is not true though.

mgreenbaum commented 8 years ago

At least I tried...

Can we pair on this to write the test?

mgreenbaum commented 8 years ago

@thangdinh Billy said you'd be the best to review this now

mgreenbaum commented 8 years ago

I've addressed the existing code review comments @billy-addepar @thangdinh

mgreenbaum commented 8 years ago

Not sure why the tests would fail here, I only changed some strings in the test (and removed some random build files that seemed unintentionally added) @billy-addepar @thangdinh

billy-addepar commented 8 years ago

retest

billy-addepar commented 8 years ago

@mgreenbaum you should run grunt dist after changing anything to update generated JS files. Otherwise, test will fail. I fixed the tests for you.

thangdinh commented 8 years ago

LGTM