Skip to content
This repository has been archived by the owner on Jun 24, 2022. It is now read-only.

Claim hooks 4 exclude expired claims #2059

Merged
merged 2 commits into from
Jan 7, 2022

Conversation

alfetopito
Copy link
Contributor

Summary

Shorter PR this time :)

Excluding the expired claim from hook useUserAvailableClaims response.
It's still possible to trigger the old behaviour by passing a flag to the hook.

To Test

  1. On claim page, check the claim for an account that has PAID claims
  • The PAID claims should not be displayed as in the current state of the contract they are expired

    Background

Optional: Give background information for changes you've made, that might be difficult to explain via comments

@alfetopito alfetopito self-assigned this Jan 7, 2022
@alfetopito alfetopito requested a review from a team January 7, 2022 00:21
@github-actions
Copy link
Contributor

github-actions bot commented Jan 7, 2022

  • 🔭 GP Swap: Gnosis Protocol v2 Swap UI

Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

Not necessarily from this PR, but this message can be confusing if you check your wallet because you claimed or it expired.

Should we broaden the message and take the two expiration dates into account?

image

EDIT: with " and take the two expiration dates into account" I'm refering to show some additional something when the investment opportunity is done.

If we want to be very specific useUserAvailableClaims could return instead of the array of claims, also 2 flags: hasExpiredClaims, claimedAmount

claimedAmount seems like convenient to show in situation of partial claims and completed claims where you see the message above.

src/custom/state/claim/hooks/index.ts Outdated Show resolved Hide resolved
@alfetopito
Copy link
Contributor Author

Not necessarily from this PR, but this message can be confusing if you check your wallet because you claimed or it expired.

Should we broaden the message and take the two expiration dates into account?

image

EDIT: with " and take the two expiration dates into account" I'm refering to show some additional something when the investment opportunity is done.

In the interest of time we decided to treat all options (no claim available, already claimed and expired claim) the same way.
Also showing people they lost an opportunity to invest might be upsetting.
Still, would be happy to add that, but only after if we have time.

If we want to be very specific useUserAvailableClaims could return instead of the array of claims, also 2 flags: hasExpiredClaims, claimedAmount

claimedAmount seems like convenient to show in situation of partial claims and completed claims where you see the message above.

This is turning the hook more complex than initially planned.
Although I do see the reason for it.

In that case, I proposed the following interface change:

  1. Do not take the flag for expired claims
  2. Return an object with 3 parameters, which are all a list of claims:
type Return = {
  availableClaims: UserClaims
  expiredClaims: UserClaims
  claimedClaims: UserClaims // hmm not the best name
}

This way the consumer can subscribe to whatever claim category it's interested on.

What do you think? @gnosis/gp-frontend

@anxolin
Copy link
Contributor

anxolin commented Jan 7, 2022

I like the proposal. And people can just ignore the arrays that they don’t need

What about:

type Return = {
  available: UserClaims
  expired: UserClaims
  claimed: UserClaims 
}

@alfetopito alfetopito merged commit efc1af0 into claim Jan 7, 2022
@alfetopito alfetopito deleted the claim-hooks-4-exclude-expire-claims branch January 7, 2022 22:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants