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

friendship page now has the option to show settled transactions #6

Merged
merged 7 commits into from
Aug 5, 2016

Conversation

Bbowen100
Copy link
Collaborator

No description provided.

let relatedObjectId = this.get('friendship.friendshipDataId')

let settledTransactions = this.get('store').filter('transaction', t => {
console.log(t.get('memo'))

Choose a reason for hiding this comment

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

Should remove console.log

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh shit ...

@@ -11,7 +11,8 @@ let Transaction = DS.Model.extend({
creator: DS.belongsTo('user'),
createdAt: DS.attr('Date', {
defaultValue: function() { return new Date() }
})
}),
is_settled: DS.attr('boolean')
Copy link
Member

@MaazAli MaazAli Jul 29, 2016

Choose a reason for hiding this comment

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

property name should be camelCase (isSettled). Also, should move it between amount and model properties.

@MaazAli
Copy link
Member

MaazAli commented Jul 29, 2016

@Bbowen100 Tested it out and it works 👍 . I did leave some comments that are mostly stylistic.

What was the need to create a separate component for the settled-list. Why couldn't we just expand the transaction-list component to accomodate for this?

Also, the current main transaction list shows all transactions, we'd like to filter that to only show un settled transactions. If users want to see settled transactions, they can open up the collapsed list.

@Bbowen100
Copy link
Collaborator Author

Bbowen100 commented Jul 29, 2016

What was the need to create a separate component for the settled-list. Why couldn't we just expand the transaction-list component to accomodate for this?

  • That is what I did originally, but I thought it would be more modular and easier to understand if I made a separate component.

Also, the current main transaction list shows all transactions, we'd like to filter that to only show un settled transactions. If users want to see settled transactions, they can open up the collapsed list.

  • On it.

…n-list, added property to differentiate between settled and unsettled transactions, changed settled-transaction button to full width of component
@@ -0,0 +1,26 @@
import { moduleForComponent, test } from 'ember-qunit';
Copy link
Member

Choose a reason for hiding this comment

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

We don't need this file anymore.

@MaazAli
Copy link
Member

MaazAli commented Aug 4, 2016

@Bbowen100 minor changes recommended. Good work on the PR.


.btn.settle-transaction {
Copy link
Member

Choose a reason for hiding this comment

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

We're using this class else where, don't need to remove it.

@MaazAli
Copy link
Member

MaazAli commented Aug 5, 2016

Looks good 👍

@MaazAli MaazAli merged commit 9f5c84e into master Aug 5, 2016
@MaazAli MaazAli deleted the feat/settled_transactions branch August 5, 2016 04:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants