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

Improve acceptance tests and Ember syntax #164

Merged
merged 9 commits into from
Jul 21, 2017
Merged

Conversation

villander
Copy link
Contributor

No description provided.

const Router = Ember.Router.extend({
const { Router } = Ember;

const EmRouter = Router.extend({
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to stay in sync with the official blueprint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kellyselden But the official blueprint has a global sweetening, why not follow the Dockyard styleguide where we reduce global calls?

https://github.com/DockYard/styleguides/blob/master/engineering/ember.md#import-what-you-use-do-not-use-globals

Copy link
Member

Choose a reason for hiding this comment

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

That styleguide will soon be out of date. This is the new importing standard: https://github.com/ember-cli/ember-rfc176-data

Copy link
Contributor Author

@villander villander Jul 10, 2017

Choose a reason for hiding this comment

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

@kellyselden Can I make that change then? From which ember-cli version is it already available? I've seen that ember-basic-dropdown is already using it, although it's still in beta.

https://github.com/cibernox/ember-basic-dropdown/blob/master/addon/components/basic-dropdown/content.js#L1

@@ -1,9 +1,12 @@
import Ember from 'ember';

export default Ember.Controller.extend({
const { Controller, A, computed } = Ember;
Copy link
Member

Choose a reason for hiding this comment

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

This is going to be outdated syntax extremely soon now that addons are starting to update to the new module api.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I'll go back to the original

assert.equal('one', find('#first-value').text());
assert.equal('two', find('#last-value').text());
assert.equal('Test Value', find('#test-input').val(), 'Has arrow functions and template string as ES6 feature');
assert.equal('one', find('#first-value').text(), 'Has generatos as ES6 feature');
Copy link
Member

Choose a reason for hiding this comment

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

generatos => generators

}
});

test('value of input', function(assert) {
test('ES6 features work correcly', function(assert) {
Copy link
Member

Choose a reason for hiding this comment

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

all these text clarifications I like.

@villander
Copy link
Contributor Author

Done @kellyselden

@villander
Copy link
Contributor Author

@kellyselden we can do merge this?

@kellyselden
Copy link
Member

I'm trying to get #167 merged first so we can add some linting rules.

@rwjblue
Copy link
Member

rwjblue commented Jul 21, 2017

#167 has landed

Copy link
Member

@stefanpenner stefanpenner left a comment

Choose a reason for hiding this comment

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

looks great!

@rwjblue rwjblue merged commit a06803d into emberjs:master Jul 21, 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.

5 participants