-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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
Update ancient emberjs version and use ember-cli #1582
Conversation
ddd2e30
to
707c437
Compare
import Ember from 'ember'; | ||
|
||
export default Ember.Controller.extend({ | ||
todos: Ember.computed.filterBy('model', 'completed', false), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left-over comma
Some things to check
|
82b2e0e
to
1221e18
Compare
@samccone Just one question. Is 4 spaces indentation a hard requirement? |
it is unless the language does not support it. I do not like it either, but consistency wins the argument here |
a559557
to
01b855b
Compare
@samccone I'm back with this, after update it to ember 2.3 final, but I'm seeing some failures in travis that don't seem related with the PR itself. Can you check the travis log to see if I'm missing something? |
I'm currently seeing CI failures here:
As far as I can tell, these aren't strictly related to the PR itself. Will keep digging. cc @samccone @passy in case they have any ideas. |
(Side: if we have time, we might want to bump to ember-cli 2.4.3 as part of this PR) |
export default Ember.Controller.extend({ | ||
repo: Ember.inject.service(), | ||
remaining: Ember.computed.filterBy('model', 'completed', false), | ||
completed: Ember.computed.filterBy('model', 'completed', true), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these two seem to be unused, or did I miss something?
re @addyosmani a rebase off of master should resolve this test failure (it is due to selenium making a breaking change) |
@samccone I'll rebase this and go to sleep. |
3b72fa7
to
d904190
Compare
324edde
to
d797874
Compare
@samccone I'm resuscitating this but I still have no idea how to make tests pass. I think that whatever the test runner is doing, it doesn't like my file structure. Who can give me a hand with this? |
d797874
to
c5a0e53
Compare
Ok, I figure this out. I'll let someone else review the code but should be good to go in a couple days top. |
startEditing() { | ||
this.get('onStartEdit')(); | ||
this.set('editing', true); | ||
Ember.run.scheduleOnce('afterRender', this, 'focusInput'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a bit hackish :\
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Afaik there is no other way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would moving it to didRender
wouldn't work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didRender
will be called whenever this component is rendered or rerendered, while this is a one-time only action. This has to happen specifically after this action and never again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
autofocus
attr doesn't work here?
24e4a1f
to
2faec97
Compare
2faec97
to
b72d5e5
Compare
This PR is done. Feel free to merge. Ember 2.6 and Ember-cli 2.6 will be out next week and I'll update this then. I intend to keep this up to date from now own and avoid it to fall far behind again. |
oh awesomeeeee 👯 we just need someone who knows ember to review this... any ideas? |
@samccone I think that Ember 2.6 is going to be released tonight, so I'll wait one more day before merging. I'll ask someone in the core team to sanction this PR. |
export default Ember.Service.extend({ | ||
lastId: 0, | ||
data: null, | ||
findAll() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a blank line above, just stylistic purposes?
💥 time to land this one :) Thanks a million @cibernox |
@samccone Do I need to create another PR to point the Ember.js version in the main page to this version instead of the 1.10 one? |
This example didn't replace the old one. It was created with a different name ( |
oh right of course :P Yeah we can switch up the link quite easily |
Maybe it could replace it tho, because saying Ember today effectively means saying ember + ember-cli. It is the officially sanctioned path. |
I am 100000% fine with removing it since the old one is very outdated. git mv FTW |
READY TO MERGE
The current version uses Ember 1.4, which is pretty ancient and the idioms are completely outdated.
This version uses Ember 2.3 and uses ember-cli as build tool.
The structure pretty different to the suggested one because of ember-cli. Ember-cli is the build took used by nearly 100% of ember projects and not using ember-cli in 2016 would be totally nuts.
I don't know if how the build system that deploys the demos works, I'll dig into it.
People in the community will review it.