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

Rewards reorder and snackbar notifications #1509

Merged
merged 8 commits into from
May 31, 2018
Merged

Rewards reorder and snackbar notifications #1509

merged 8 commits into from
May 31, 2018

Conversation

daovist
Copy link
Contributor

@daovist daovist commented May 23, 2018

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

@tzarebczan
Copy link
Contributor

@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.

@daovist
Copy link
Contributor Author

daovist commented May 24, 2018

@tzarebczan as I cleaned up rewards.js I realized I was duplicating the reward notification so that is fixed now too

Copy link

@neb-b neb-b left a 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) {
Copy link

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?

Copy link
Contributor Author

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

Copy link
Contributor

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

Copy link
Contributor Author

@daovist daovist left a 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) {
Copy link
Contributor Author

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

Copy link
Contributor

@skhameneh skhameneh left a 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));
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor Author

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.
@tzarebczan
Copy link
Contributor

Giving this a test with the new user reward.

@tzarebczan
Copy link
Contributor

tzarebczan commented May 30, 2018

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.

@tzarebczan
Copy link
Contributor

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.

@neb-b neb-b merged commit d65d92e into master May 31, 2018
@neb-b neb-b deleted the issue/1329 branch May 31, 2018 04:46
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.

4 participants