-
Notifications
You must be signed in to change notification settings - Fork 642
Use "New Module Imports" #871
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
Conversation
import loadInitializers from 'ember-load-initializers'; | ||
|
||
import Resolver from './resolver'; |
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 put resolver before loadInitializers? or did the order change in blueprints?
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.
my convention is usually to import external packages first before importing local files. since ember-load-initializers
is external and ./resolver
is a local file the resolver is imported afterwards
app/components/api-token-row.js
Outdated
@@ -1,8 +1,9 @@ | |||
import Ember from 'ember'; | |||
import { empty, or } from '@ember/object/computed'; | |||
import Component from '@ember/component'; |
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.
Component first
app/components/badge-appveyor.js
Outdated
tagName: 'span', | ||
classNames: ['badge'], | ||
repository: Ember.computed.alias('badge.attributes.repository'), | ||
branch: Ember.computed('badge.attributes.branch', function() { | ||
repository: alias('badge.attributes.repository'), |
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.
we might want to introduce some newlines here, for readability
tagName: '', | ||
|
||
didInsertElement() { | ||
Ember.$.getScript('https://www.google.com/jsapi', function() { | ||
$.getScript('https://www.google.com/jsapi', function() { |
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.
we won't have a problem with these?
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.
I don't think so, why?
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.
hm, I guess these will be short lived anyway, as we'll want to use something fastboot-compatible.
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.
sure, but replacing $.getScript()
is unrelated to the purpose of this PR
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.
I know, I'm just saying it won't matter soon enough :P
see Ember.js RFC rust-lang#176
b7581ba
to
29e9fe2
Compare
see Ember.js RFC #176 and https://github.com/ember-cli/ember-rfc176-data