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.

Sign up for GitHub

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

Jump to bottom

#588 - octane - guides update - model/relationships #634

Merged
merged 2 commits into from
Apr 6, 2019
Merged

#588 - octane - guides update - model/relationships #634

merged 2 commits into from
Apr 6, 2019

Conversation

dennismende
Copy link
Contributor

@dennismende dennismende commented Mar 17, 2019 •
edited
Loading

No description provided.

@dennismende
Copy link
Contributor Author

removed computed and make use of getter to stick to decisions made in #543 and #553

@locks
Copy link
Contributor

locks commented Mar 17, 2019

Hm, I believe these should be tracked properties, otherwise it won't update? cc @pzuraq

@mixonic
Copy link
Contributor

mixonic commented Mar 18, 2019

I think @pzuraq was suggesting this very change in #553 (comment).

@pzuraq
Copy link
Contributor

pzuraq commented Mar 18, 2019

Yup! So, @attr is a computed property, which means that when it is encountered by the autotracker it will be autotracked. So we can convert any CPs that rely on that property to standard getters instead 😄 any non @attrs/computeds do still need to be converted to tracked properties, but that doesn't apply in this case

jamescdavis
jamescdavis suggested changes Mar 18, 2019
View reviewed changes
Copy link
Contributor

@jamescdavis jamescdavis left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Also in PaymentPethodPaypal, but github won't let me comment on that line.

guides/release/models/relationships.md
import PaymentMethod from './payment-method';
const { attr } = DS;

export default class PaymentMethodCc extends PaymentMethod {
@attr() last4;

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

Choose a reason for hiding this comment

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

Suggested change
})
}

@jenweber
Copy link
Contributor

@dennismende Could you make the two punctuation fixes noted by James (}) should be ) in two places)? Then this will be good to merge!

It is helpful if you check "allow maintainers to make changes" when you open PRs, if you are ok with it. It's not required, but it means we can do things like update stale branches or fix small typos without bugging you about it. Thanks for the help!

@jenweber jenweber mentioned this pull request Apr 6, 2019
Merged
@jenweber jenweber merged commit b6985e9 into ember-learn:octane Apr 6, 2019
@jenweber
Copy link
Contributor

jenweber commented Apr 6, 2019

I pulled these commits by @dennismende into a fresh PR, and then made the typo fixes. It's been merged! Thanks Dennis for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jamescdavis jamescdavis jamescdavis requested changes

Assignees
No one assigned
Labels
needs contributor response
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dennismende @locks @mixonic @pzuraq @jenweber @jamescdavis