-
Notifications
You must be signed in to change notification settings - Fork 4
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
Additional reward cards for point history #93
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work Tommy! A couple of cleanup + reordering comments, but then I think it's good to go 👍
</Caption> | ||
<Subhead>{`${pointsEarned} points earned`}</Subhead> | ||
<Caption color={Colors.secondaryText}> | ||
{`for ${displayDollarValue(totalSale || 0)} of healthy products`} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it was like this before but would this make more sense to be subtotal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh nevermind I guess it makes sense that the number of points directly corresponds to the total sale (bc the rewards points are deducted?) but this language might not fit since they did actually pay more for the total healthy products...if that makes sense
cc @12aschen ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably easiest to match what we do in Clerk (which I cannot currently recall) but I would guess this is a quacetion that we can defer. Just don't forget to check in on it 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!! (': One thing before merge - didn't we need to update the rewardPointValue
to 1500 in both repos?
What's new in this PR
Relevant Links
Related PRs
How to review
Next steps
Tests Performed, Edge Cases
Screenshots
CC: @anniero98 @wangannie