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

feat: add claim button to header #2472

Merged
merged 6 commits into from
Aug 31, 2023
Merged

feat: add claim button to header #2472

merged 6 commits into from
Aug 31, 2023

Conversation

iamacook
Copy link
Member

@iamacook iamacook commented Aug 30, 2023

What it solves

Implements claim button in header

How this PR fixes it

The token allocation element in the header now includes a "New allocation" button according to the designs. Clicking it will take you to the claiming app and, providing there is nothing to claim, it should not appear.

How to test it

Open a Safe with an allocation and observe the new button. Claiming should make it dissappear and opening a Safe without an allocation should not show it at all.

Clicking on the button should the following event: "Click on SEP5 allocation button".

Screenshots

image

image

Checklist

  • I've tested the branch on mobile 📱
  • I've documented how it affects the analytics (if at all) 📊
  • I've written a unit/e2e test for it (if applicable) 🧑‍💻

@iamacook iamacook requested review from katspaugh and schmanu August 30, 2023 08:34
@iamacook iamacook self-assigned this Aug 30, 2023
@github-actions
Copy link

github-actions bot commented Aug 30, 2023

Branch preview

✅ Deploy successful!

https://sep5_header--walletweb.review-wallet-web.5afe.dev

@github-actions
Copy link

github-actions bot commented Aug 30, 2023

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@@ -126,30 +142,26 @@ const fetchTokenBalance = async (chainId: string, safeAddress: string): Promise<
* The Safe token allocation is equal to the voting power.
* It is computed by adding all vested tokens - claimed tokens + token balance
*/
const useSafeTokenAllocation = (): [BigNumber | undefined, boolean] => {
export const useSafeTokenBalance = (allocationData?: Vesting[]): AsyncResult<BigNumber> => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export const useSafeTokenBalance = (allocationData?: Vesting[]): AsyncResult<BigNumber> => {
export const useSafeVotingPower = (allocationData?: Vesting[]): AsyncResult<BigNumber> => {

Should we rename it to useSafeVotingPower?
useSafeTokenBalance sounds to me as if it would just fetch the current balance in a hook.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've renamed it accordingly in ba34f38.

return false
}

return !sep5Allocation.isRedeemed
Copy link
Member

Choose a reason for hiding this comment

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

Like this we would need to make a change once the deadline is reached.
I think also checking if the allocation is expired would fix that.

Suggested change
return !sep5Allocation.isRedeemed
return !sep5Allocation.isRedeemed && !sep5Allocation.isExpired

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added the extra check in ba34f38.

@iamacook iamacook requested a review from usame-algan August 30, 2023 09:26
Copy link
Member

@usame-algan usame-algan left a comment

Choose a reason for hiding this comment

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

Looks good 👍 I've tested this with my görli safes.

  • The one that is elligible for sep5 had the new button in the header. After redeeming, the button disappears
  • The one that is not elligible for sep5 has no button in the header

@iamacook iamacook requested a review from katspaugh August 30, 2023 11:17
@francovenica
Copy link
Contributor

francovenica commented Aug 30, 2023

My only issue is the position of the green dot. I think it should be on the right bottom corner of the widget instead of being in the middle:
image

maybe something like this:
image

The rest is fine:
The button won't show if you don't have tokens to claim or if you already claimed the tokens
The tracking works fine as well
image

If the position of the green dot is not a problem for the rest of the team we can pass the ticket to done

@iamacook
Copy link
Member Author

My only issue is the position of the green dot. I think it should be on the right bottom corner of the widget instead of being in the middle

If the position of the green dot is not a problem for the rest of the team we can pass the ticket to done

We discussed this and deemed it fine as is.

@iamacook iamacook merged commit 90c5423 into dev Aug 31, 2023
@iamacook iamacook deleted the sep5-header branch August 31, 2023 08:06
@github-actions github-actions bot locked and limited conversation to collaborators Aug 31, 2023
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.

5 participants