adopted-ember-addons / ember-cli-flash

Simple, highly configurable flash messages for ember-cli
https://www.npmjs.com/package/ember-cli-flash
MIT License
355 stars 113 forks source link

Upgraded to ember 2.14.1 #236

Closed bl4ckm0r3 closed 7 years ago

bl4ckm0r3 commented 7 years ago

slightly changed tests to use wait

@poteto i had to change the sinon test, the component was actually calling the callback but sinon wasn't recognizing it (maybe the timer wasn't working?)

Dhaulagiri commented 7 years ago

i had to change the sinon test

I think this may be fixed in Ember 2.15. We should be able to confirm once that gets released soon.

bl4ckm0r3 commented 7 years ago

@Dhaulagiri i'll wait until 2.15 is out and we'll think about a strategy (probably close this and open a new pr?)

Dhaulagiri commented 7 years ago

@bl4ckm0r3 I don't have a strong opinion. The main thing I want to do is get tests passing on master again and then we can probably use most of what is in this PR (minus the test change) and follow-on with a separate PR to update to ember-cli/ember 2.15 specifically. If ember 2.15 does fix the reason our tests are failing we will get that "fix" for free since ember try will automatically run tests against it.

Dhaulagiri commented 7 years ago

It doesn't look like ember 2.15 has changed anything with regard to the test failure, so something like what we have here seems necessary although I admit I'm not 100% familiar with the code to confidently give it a 👍 .

sbatson5 commented 7 years ago

@Dhaulagiri the intent of the test was to show that hovering over a message paused the dismissal timer (i.e. we could hover, wait an amount of time and test that our message was still there).

As @cmcclure pointed out in https://github.com/poteto/ember-cli-flash/issues/237, the problem/solution may have been over engineered (that's on me). If we go the route of not having the timer paused, but instead just keep the message from destroying until mouse-leave, we should be able to go with these test updates.

bl4ckm0r3 commented 7 years ago

sorry for the delay i took some days off! Have updated the pr, let me know what you think! thanks

sbatson5 commented 7 years ago

@bl4ckm0r3 going to merge this in and PR another couple updates to fix the broken tests (larger issue unrelated to your updates)