-
Notifications
You must be signed in to change notification settings - Fork 414
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
Rewards reorder and snackbar notifications #1509
Conversation
@daovist we should clean up: https://github.com/lbryio/lbry-app/blob/master/src/renderer/rewards.js right? Since it's no longer considering those types and reward messages. |
@tzarebczan as I cleaned up |
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.
Should we move the claimedRewards over to an array to be consistent? Not really sure.
@@ -61,13 +61,6 @@ export function doClaimRewardType(rewardType) { | |||
reward: successReward, | |||
}, | |||
}); | |||
if (successReward.reward_type === rewards.TYPE_NEW_USER) { |
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.
Why are we getting rid of this?
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.
The new_user
reward notification is handled by rewards.js
like the others
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.
@daovist there should be a modal (in addition to snackbar) for first reward earned - if it doesn't belong in this code, needs to be added to https://github.com/lbryio/lbry-app/pull/1509/files#diff-ecd3ac4e31248b9eede5ec309f95d368
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 don't think it matters much if we refactor claimedRewards but am happy to do so
@@ -61,13 +61,6 @@ export function doClaimRewardType(rewardType) { | |||
reward: successReward, | |||
}, | |||
}); | |||
if (successReward.reward_type === rewards.TYPE_NEW_USER) { |
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.
The new_user
reward notification is handled by rewards.js
like the others
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.
Minor comment
const unclaimedRewardsByType = Object.assign({}, state.unclaimedRewardsByType); | ||
const existingReward = unclaimedRewardsByType[reward.reward_type]; | ||
const index = unclaimedRewards.findIndex(ur => ur.reward_type === reward.reward_type); | ||
unclaimedRewards = unclaimedRewards.slice(0, index).concat(unclaimedRewards.slice(index + 1)); |
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 looks like you're removing the current element?
I would suggest using splice instead.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/splice
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 tried changing unclaimedRewards = unclaimedRewards.slice(0, index).concat(unclaimedRewards.slice(index + 1));
to unclaimedRewards.splice(index, 1);
and the UI no longer updates properly. It removes the <RewardLink>
button but not the Card with the title and info. I ran into this last week but moved on when I got it working correctly with concat. I am still working to grok the finer points of JS/Redux mutability.
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.
That's likely because it's modifying the original array value and React only shallow checks.
If you return [...unclaimedRewards]
instead of unclaimedRewards
, it'll force the update.
This is a good use case for immutable.js as well
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.
Thanks. I did the same with claimed rewards and now they're showing up at the bottom like they should.
…isplay reward notification message through snackbar with fallback for rewards without a message.
Giving this a test with the new user reward. |
The last cleanup of rewards.js broke reward.type constant comparisons which breaks first reward modal and ability to claim rewards like publish / channel. Travis looking into it. |
This is working as expected again. @seanyesmunt we also removed the amount from the invite button because it was coming back as 0. It may be misleading also, because sending the invite doesn't actually get you 3 credits (the person has to sign up, get approved, etc), so I think it's better this way. |
Refactor unclaimed rewards as array to keep order from api.lbry.io. Display reward notification message through snackbar with fallback for rewards with no message.
#1329