Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgraded to ember 2.14.1 #236

Merged

Conversation

bl4ckm0r3
Copy link
Contributor

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?)

@bl4ckm0r3 bl4ckm0r3 force-pushed the update-to-ember-2.14.1 branch 2 times, most recently from b733da9 to 1aaa1ed Compare July 28, 2017 17:55
Copy link
Collaborator

@Dhaulagiri Dhaulagiri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! I think we can land this with a few small updates:

  • Can you rebase against master and run yarn to update yarn.lock with your package.json changes?
  • Can you delete .jshintrc and tests/.jshintrc (and remove any other jshint references you can find?

@Dhaulagiri
Copy link
Collaborator

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
Copy link
Contributor Author

@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
Copy link
Collaborator

Dhaulagiri commented Aug 21, 2017

@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.

bower.json Outdated
@@ -1,8 +1,6 @@
{
"name": "ember-cli-flash",
"dependencies": {
"ember": "~2.7.0",
"ember-cli-shims": "0.1.1",
"ember-qunit-notifications": "0.1.0",
"sinonjs": "^1.17.1"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm pretty sure this can be removed as ember-sinon doesn't need this as of 0.5.1

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to move away from bower entirely with a few updates.

bower.json Outdated
@@ -1,8 +1,6 @@
{
"name": "ember-cli-flash",
"dependencies": {
"ember": "~2.7.0",
"ember-cli-shims": "0.1.1",
"ember-qunit-notifications": "0.1.0",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can also be removed once ember-cli-qunit gets updated as it is in this PR. We can do a separate PR to remove all the bower cruft.

@@ -0,0 +1,16 @@
module.exports = {
Copy link
Collaborator

@Dhaulagiri Dhaulagiri Aug 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the tabs in this file should only be 2 characters, not 4

@Dhaulagiri
Copy link
Collaborator

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
Copy link
Collaborator

@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 #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.

slightly changed tests to use wait
removed jshint references
removed bower
added yarn
@bl4ckm0r3 bl4ckm0r3 force-pushed the update-to-ember-2.14.1 branch from 39b935d to 3fe2b34 Compare September 5, 2017 13:28
@bl4ckm0r3
Copy link
Contributor Author

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

@bl4ckm0r3 bl4ckm0r3 force-pushed the update-to-ember-2.14.1 branch from 148ab7f to 066f70b Compare September 5, 2017 13:42
@sbatson5
Copy link
Collaborator

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

@sbatson5 sbatson5 merged commit c0098d2 into adopted-ember-addons:master Sep 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants