Skip to content

Conversation

@BoulderBrains
Copy link
Contributor

No description provided.

@BoulderBrains BoulderBrains requested a review from mcstover March 20, 2019 19:49
methods: {
toggleFavorite() {
this.$emit('favorite-toggled');
if (this.isFavorite === false) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Spoke with @BoulderBrains about incorporating this into GridLoanCard and LoanCardController instead.

toggleFavorite() {
// optimistically toggle it locally first
this.isFavorite = !this.isFavorite;
// this.isFavorite = !this.isFavorite;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Prob should keep this. It sets the "optimistic value" and immediately shows the new expected star state.

});
} else {
this.$kvTrackEvent(
'favorited',
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be "Lending" to put it into the context of other lending flow events.

_forEach(data.errors, ({ message }) => {
this.$showTipMsg(message, 'error');
});
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You probably want to also set this.isFavorite just inside this else statement using the return value from your mutation "data".

}).catch(error => {
console.error(error);
});
this.isFavorite = !this.isFavorite;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this won't be necessary with the changes mentioned above.

… moving into GridLoanCard and LoanCardController
@BoulderBrains
Copy link
Contributor Author

Alright, @mcstover I've made some changes after our discussion today and this is now working again. A couple things I still want to talk through with you.

  1. The comments I left on lines 172&173 of GridLoanCard.vue. That toggle really feels like it should happen only after we're not getting an error response from graphql.

  2. I've noticed in some situations we have the same loan card rendered on the page twice. WHen I favorite one of those loans, the GridLoanCard I clicked on to favorite updates, but the other copy of the same card stays unchecked until the page refreshes.

@BoulderBrains
Copy link
Contributor Author

This new branch has been created for the improvement of the favorite functionality work that we discussed through the course of this work: https://github.com/kiva/ui/tree/work-toward-improved-favoriting

@BoulderBrains BoulderBrains merged commit 3a37402 into master Mar 22, 2019
@BoulderBrains BoulderBrains deleted the cash-513 branch March 22, 2019 21:26
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