Skip to content
This repository has been archived by the owner on May 26, 2019. It is now read-only.

Use es5 getters #2305

Closed

Conversation

kennethlarsen
Copy link
Member

@kennethlarsen kennethlarsen commented Mar 22, 2018

Issue reference: #2303

This PR changes use of get() to es5 getters in places where it makes sense according to the 3.1 release.

Sadly, since I am not a human codemod there might be errors here or areas I have overseen so a sanity check would be nice.

@kennethlarsen kennethlarsen force-pushed the feature/use-es5-getters branch from 864478d to 90bd58c Compare March 23, 2018 13:09
kennethlarsen pushed a commit to kennethlarsen/guides that referenced this pull request Mar 23, 2018
@kennethlarsen kennethlarsen changed the title [WIP]: Use es5 getters Use es5 getters Mar 23, 2018
@@ -250,7 +250,7 @@ export default Component.extend({
}),

click() {
let player = this.get('audioService');
let player = this.audioService;
player.play(this.get('song.file'));
Copy link
Member

Choose a reason for hiding this comment

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

this could probably be simplified to this.audioService.play(...) now :)

obfuscatedIdentifier: computed('last4', function () {
return `**** **** **** ${this.get('last4')}`;
return `**** **** **** ${this.last4}`;
Copy link
Member

Choose a reason for hiding this comment

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

I'm unsure about this one as it involves Ember Data models. I think we might have to keep using get() here for now. 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe as long as last4 isn't a relationship we should be fine, because that's where the proxies are.

@@ -25,7 +25,7 @@ export default Component.extend({
},

titles: computed('todos.[]', function() {
let todos = this.get('todos');
let todos = this.todos;
return todos.mapBy('title');
Copy link
Member

Choose a reason for hiding this comment

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

like above, these can now be simplified to e.g. return this.todos.mapBy('title')

@kennethlarsen kennethlarsen force-pushed the feature/use-es5-getters branch from 90bd58c to c89fc4b Compare March 23, 2018 17:36
mansona pushed a commit that referenced this pull request Apr 21, 2018
@mansona mansona mentioned this pull request Apr 21, 2018
@sivakumar-kailasam
Copy link
Member

Thanks for taking the time to contribute to this project. We’re in the process of switching from middleman to an ember app to make it easy for contribution.

We’re closing issue since its been open for too long without progress / it needs more discussion which is probably better had via a new PR in the new app. (sorry about that)

If this PR has changes to the content of guides please raise a PR to https://github.com/ember-learn/guides-source. If the change is to the guides viewer (i.e., the app) then please raise a PR in https://github.com/ember-learn/guides-app/. The READMEs and contributing.md guidelines of those projects will familiarize you with the new process. Let us know if you have any questions!

PS: this is a standard msg we're using across such PRs

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants