Add setApprovalForAll confirmation view #15010
Merged
+154
−16
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
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. |
adonesky1
force-pushed
the
fix-set-approval-for-all
branch
5 times, most recently
from
48cac45 to
aa76f84
Compare
adonesky1
changed the title
Collaborator
|
This looks good to me. Big improvement to what we currently have 🚢 |
digiwand
previously approved these changes
Jun 29, 2022
Contributor
digiwand
left a comment
digiwand
left a comment
There was a problem hiding this comment.
Nice fix / feature update!
No blockers, just some small comments to consider later
| title = t('revokeAllTokensTitle', [titleTokenDescription]); | ||
| } | ||
| } | ||
| return title; |
Contributor
There was a problem hiding this comment.
comment & no need to change:
Another way to go about this is
let title;
// ...
return title || t('allowSpendToken', [titleTokenDescription]);
could save 1 translation call
| {t('approvedAsset')}: | ||
| </div> | ||
| <div className="confirm-approve-content__medium-text"> | ||
| {isSetApproveForAll ? `${t('allOfYour')} ` : null} |
Contributor
There was a problem hiding this comment.
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
| ? t('functionSetApprovalForAll') | ||
| : t('functionApprove')} | ||
| </div> | ||
| <div className="confirm-approve-content__small-text"> |
Contributor
There was a problem hiding this comment.
nit and non-blocker: Could avoid showing an empty element when we are not displaying isSetApproveForAll
Contributor
|
verified using Chrome x MacOS |
danjm
reviewed
Jun 30, 2022
| title = t('approveSpendLimit', [token?.symbol || t('token')]); | ||
| subtitle = origin; | ||
| subtitleContainsOrigin = true; | ||
| } else if (type === TRANSACTION_TYPES.TOKEN_METHOD_SET_APPROVAL_FOR_ALL) { |
Contributor
There was a problem hiding this comment.
does this type actually get set anywhere?
Contributor
There was a problem hiding this comment.
@digiwand when you tested this, did you see the setApprovalForAllTitle in the transaction history list?
Contributor
Author
There was a problem hiding this comment.
Should be able to! I see it. cc @digiwand
Contributor
Author
There was a problem hiding this comment.
its set here in the method determineTransactionType
adonesky1
force-pushed
the
fix-set-approval-for-all
branch
from
aa76f84 to
2584852
Compare
Collaborator
Builds ready [2584852]Page Load Metrics (1729 ± 42 ms)
highlights:storybook
|
georgewrmarshall
approved these changes
Jul 11, 2022
darkwing
approved these changes
Jul 11, 2022
| const { isSetApproveForAll, setApproveForAllArg } = this.props; | ||
| const titleTokenDescription = this.getTitleTokenDescription(); | ||
|
|
||
| let title; |
Contributor
There was a problem hiding this comment.
This could be let title = t('allowSpendToken', [titleTokenDescription]);, and we avoid the || in the return statement, but I won't block on that.
Contributor
Author
There was a problem hiding this comment.
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.
Contributor
There was a problem hiding this comment.
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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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-approvecomponent 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:
setApproveForAll(when revoking permission):Before:
After:
How to test:
contracttab and thenwrite contract:
3. Connect your wallet by clicking `Connect to Web3`
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)