LBRYCommunity/lbry-desktop

Fork 0

Rewards reorder and snackbar notifications #1509

Merged

daovist merged 8 commits from issue/1329 into master

2018-05-31 06:46:52 +02:00

daovist commented

2018-05-23 20:46:13 +02:00

(Migrated from github.com)

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

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 commented

2018-05-24 16:03:16 +02:00

(Migrated from github.com)

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

2018-05-24 21:51:23 +02:00

(Migrated from github.com)

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

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

neb-b (Migrated from github.com) reviewed 2018-05-25 05:32:48 +02:00

neb-b (Migrated from github.com) left a comment

Should we move the claimedRewards over to an array to be consistent? Not really sure.

src/renderer/redux/actions/rewards.js

neb-b (Migrated from github.com) commented

2018-05-25 05:31:16 +02:00

Why are we getting rid of this?

daovist (Migrated from github.com) reviewed 2018-05-25 16:06:30 +02:00

daovist (Migrated from github.com) left a comment

I don't think it matters much if we refactor claimedRewards but am happy to do so

src/renderer/redux/actions/rewards.js

daovist (Migrated from github.com) commented

2018-05-25 16:04:46 +02:00

The new_user reward notification is handled by rewards.js like the others

The `new_user` reward notification is handled by `rewards.js` like the others

skhameneh (Migrated from github.com) approved these changes 2018-05-29 06:50:23 +02:00

skhameneh (Migrated from github.com) left a comment

Minor comment

src/renderer/redux/reducers/rewards.js

skhameneh (Migrated from github.com) commented

2018-05-29 06:49:02 +02:00

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

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

daovist (Migrated from github.com) reviewed 2018-05-29 20:20:14 +02:00

src/renderer/redux/reducers/rewards.js

daovist (Migrated from github.com) commented

2018-05-29 20:20:13 +02:00

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.

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.

tzarebczan (Migrated from github.com) reviewed 2018-05-29 20:40:29 +02:00

src/renderer/redux/actions/rewards.js

tzarebczan (Migrated from github.com) commented

2018-05-29 20:40:29 +02:00

@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

tzarebczan commented

2018-05-29 23:18:16 +02:00

(Migrated from github.com)

Giving this a test with the new user reward.

skhameneh (Migrated from github.com) reviewed 2018-05-30 00:49:46 +02:00

src/renderer/redux/reducers/rewards.js

skhameneh (Migrated from github.com) commented

2018-05-30 00:49:46 +02:00

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

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

tzarebczan commented

2018-05-30 03:01:19 +02:00

(Migrated from github.com)

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.

daovist (Migrated from github.com) reviewed 2018-05-30 20:06:42 +02:00

src/renderer/redux/reducers/rewards.js

daovist (Migrated from github.com) commented

2018-05-30 20:06:42 +02:00

Thanks. I did the same with claimed rewards and now they're showing up at the bottom like they should.

tzarebczan commented

2018-05-31 02:29:22 +02:00

(Migrated from github.com)

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.