-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Add setApprovalForAll confirmation view #15010
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
48cac45
to
aa76f84
Compare
This looks good to me. Big improvement to what we currently have 🚢 |
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.
Nice fix / feature update!
No blockers, just some small comments to consider later
title = t('revokeAllTokensTitle', [titleTokenDescription]); | ||
} | ||
} | ||
return title; |
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.
comment & no need to change:
Another way to go about this is
let title;
// ...
return title || t('allowSpendToken', [titleTokenDescription]);
could save 1 translation call
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.
Done here: 2584852
@@ -201,6 +203,7 @@ export default class ConfirmApproveContent extends Component { | |||
{t('approvedAsset')}: | |||
</div> | |||
<div className="confirm-approve-content__medium-text"> | |||
{isSetApproveForAll ? `${t('allOfYour')} ` : null} |
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.
This is not a blocker for this fix. We should pass this in as a param to the titleTokenDescription
because the translations might change the sentence structure
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.
Done here: 2584852
? t('functionSetApprovalForAll') | ||
: t('functionApprove')} | ||
</div> | ||
<div className="confirm-approve-content__small-text"> |
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.
nit and non-blocker: Could avoid showing an empty element when we are not displaying isSetApproveForAll
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.
Done here: 2584852
verified using Chrome x MacOS |
@@ -222,6 +222,12 @@ export function useTransactionDisplayData(transactionGroup) { | |||
title = t('approveSpendLimit', [token?.symbol || t('token')]); | |||
subtitle = origin; | |||
subtitleContainsOrigin = true; | |||
} else if (type === TRANSACTION_TYPES.TOKEN_METHOD_SET_APPROVAL_FOR_ALL) { |
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.
does this type actually get set anywhere?
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.
@digiwand when you tested this, did you see the setApprovalForAllTitle
in the transaction history list?
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 be able to! I see it. cc @digiwand
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.
its set here in the method determineTransactionType
aa76f84
to
2584852
Compare
Builds ready [2584852]Page Load Metrics (1729 ± 42 ms)
highlights:storybook
|
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.
UI looks good to me! I can see some improvements that we can make to approval screen UI in general which I can save for another PR
const { isSetApproveForAll, setApproveForAllArg } = this.props; | ||
const titleTokenDescription = this.getTitleTokenDescription(); | ||
|
||
let title; |
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.
This could be let title = t('allowSpendToken', [titleTokenDescription]);
, and we avoid the ||
in the return statement, but I won't block on that.
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.
lol this is what I had before and Ariella suggested this way: #15010 (comment)
I don't have much of an opinion on this one.
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.
haha sorry @adonesky1 and @darkwing. I actually liked the ||
to possibly do one less translation here. I can switch the preference to @darkwing 's for future implementations
Fixes: #15070
I haven't had time or any design feedback to create a wholly separate flow specific to
setApproveForAll
, rather I've further parameterized theconfirm-approve
component to provide specific verbiage for this method (both granting and revoking approval). This is not the way I would do it with more time, but the timeline has been compressed and there is some urgency to get something out there since this method is so commonly used.cc @rachelcope and/or @SayaGT we may want to tweak the verbiage here.
To Do after this PR:
setApproveForAll
(when granting permission)Before:
After:
![Screen Shot 2022-06-28 at 4 05 34 PM](https://user-images.githubusercontent.com/34557516/176295985-be7915aa-c914-473d-a458-8d6869849aec.png)
![Screen Shot 2022-06-28 at 4 04 57 PM](https://user-images.githubusercontent.com/34557516/176296162-d230b27c-70e5-4a01-9dcb-3f9580960bd8.png)
setApproveForAll
(when revoking permission):Before:
![Screen Shot 2022-06-28 at 4 36 05 PM](https://user-images.githubusercontent.com/34557516/176302871-a48b3aa7-a854-40ce-8902-ccb4b4ac2b3a.png)
After:
![Screen Shot 2022-06-28 at 4 25 50 PM](https://user-images.githubusercontent.com/34557516/176300684-6ec21643-ff8d-445b-8fee-c772eb673c01.png)
![Screen Shot 2022-06-28 at 4 26 24 PM](https://user-images.githubusercontent.com/34557516/176300731-cc186846-8830-488a-9831-ba7694106736.png)
How to test:
contract
tab and thenwrite contract
:![Screen Shot 2022-06-28 at 4 37 02 PM](https://user-images.githubusercontent.com/34557516/176303112-4e606385-8bd6-4a25-92d9-e23022beb3a6.png)
3. Connect your wallet by clicking `Connect to Web3`![Screen Shot 2022-06-28 at 4 38 08 PM](https://user-images.githubusercontent.com/34557516/176303480-cd4ac719-116c-428f-97a6-177f9f343785.png)
4. Scroll down to `setApprovalForAll` and click it so it drops down inputs 5. Enter args: For `operator` enter another address (any will do) and for `approved` enter true to see the grant permission flow. (then repeat and enter false to see the revoke flow)