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

[EMBER]: upgrade to 2.8 and remove DS check #215

Merged
merged 1 commit into from
Apr 17, 2017
Merged

[EMBER]: upgrade to 2.8 and remove DS check #215

merged 1 commit into from
Apr 17, 2017

Conversation

snewcomer
Copy link
Collaborator

@snewcomer snewcomer commented Apr 12, 2017

References #214

  • remove ember data check. If user is not using ember-data, this check will fail?
  • Update tests to use moduleForAcceptance
  • Add docs for a few methods
  • remove ember-version-is
  • add test for custom store

@snewcomer snewcomer self-assigned this Apr 12, 2017
@snewcomer
Copy link
Collaborator Author

snewcomer commented Apr 15, 2017

Running this test here and seems as though in test land Ember's event handling has changed and for some reason the height/distance of items in the DOM are also different. Need to figure out where/why this has changed to be able to modify the tests.

master ember 2.4 ember-cli-qunit <= 2.1
screen shot 2017-04-15 at 3 15 58 pm

this branch ember 2.8 ember-cli-qunit > 2.1
screen shot 2017-04-15 at 3 19 42 pm

@snewcomer
Copy link
Collaborator Author

snewcomer commented Apr 16, 2017

Ok so what is interesting is upgrading from ember-cli-qunit 1.4 to 2.1 is ok. ember-cli-qunit 2.2 is what causes some scroll tests to fail - ^^prev comment - in acceptance land. Now will dig into that.

@snewcomer
Copy link
Collaborator Author

snewcomer commented Apr 17, 2017

@hhff this PR is passing now. A few other things other than upgrading are listed above. Will look into the ember-cli-qunit change that causes the difference above, but we could merge in as is.

@@ -4,7 +4,9 @@ var EmberAddon = require('ember-cli/lib/broccoli/ember-addon');

module.exports = function(defaults) {
var app = new EmberAddon(defaults, {
// Add options here
babel: {
Copy link
Collaborator Author

@snewcomer snewcomer Apr 17, 2017

Choose a reason for hiding this comment

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

Take care of Object assign not existing in < ie 11?

@hhff
Copy link
Collaborator

hhff commented Apr 17, 2017

@snewcomer - in the past I've done const assign = Object.assign || Ember.assign. Is that simpler?

@hhff
Copy link
Collaborator

hhff commented Apr 17, 2017

@snewcomer - right now, a user can't use Ember Infinity without Ember Data (this.store.find or this.store.query are both ED methods)

@hhff
Copy link
Collaborator

hhff commented Apr 17, 2017

re: Ember Data - I would like to remove the dep on Ember-Version-Is however...

@snewcomer
Copy link
Collaborator Author

snewcomer commented Apr 17, 2017

@hhff I think the case for the util is if <=ie11 and no Ember.assign. Also here is an acceptance test using a custom store and custom find method :). Let me know your thoughts.

450a142

https://github.com/hhff/ember-infinity/pull/215/files#diff-81e017c2c36f4a656f27bccf04c2fb31R1

@hhff
Copy link
Collaborator

hhff commented Apr 17, 2017

Awesome work. I'm cool with all of this. Ok for me to merge?

@snewcomer snewcomer changed the title WIP: start upgrade to 2.8 and remove DS check [EMBER]: upgrade to 2.8 and remove DS check Apr 17, 2017
@snewcomer
Copy link
Collaborator Author

@hhff yep if you are good with these changes!

@hhff hhff merged commit 74da0ad into master Apr 17, 2017
@hhff hhff deleted the upgrade-2.8 branch April 17, 2017 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants