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.

Sign up for GitHub

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

Jump to bottom

Patch network specific asset modal (Token detection) #3980

Merged
merged 5 commits into from
Apr 15, 2022
Merged

Patch network specific asset modal (Token detection) #3980

merged 5 commits into from
Apr 15, 2022

Conversation

blackdevelopa
Copy link
Contributor

Description

Patched network specific assets education modal to include token detection (mainnet)

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

Issue

Resolves #???

@blackdevelopa blackdevelopa requested a review from a team as a code owner March 30, 2022 09:50
@github-actions
Copy link
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.

@blackdevelopa blackdevelopa self-assigned this Mar 30, 2022
@blackdevelopa blackdevelopa added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Mar 30, 2022
@owencraston
Copy link
Contributor

User testing results

@owencraston
Copy link
Contributor

Here we can see when token detection is on, the asset education modal messaging is different, indicating that tokens should be auto imported.
https://user-images.githubusercontent.com/22918444/160901309-887b2236-24f1-42cb-84f6-1f50614706c8.mov

  • From a UI perspective everything looks good, just left a few comments about the code

@blackdevelopa blackdevelopa force-pushed the patch-token-detection/network-education-modal branch from 5660a2a to 1551b0a Compare March 31, 2022 17:27
@blackdevelopa blackdevelopa added needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Apr 1, 2022
@chrisleewilcox chrisleewilcox added QA in Progress QA has started on the feature. and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Apr 12, 2022
@chrisleewilcox
Copy link
Contributor

Getting the following error when running yarn setup...

**ERROR** Failed to apply patch for package @metamask/design-tokens at path
  
    node_modules/@metamask/design-tokens

  This error was caused because @metamask/design-tokens has changed since you
  made the patch file for it. This introduced conflicts with your patch,
  just like a merge conflict in Git when separate incompatible changes are
  made to the same piece of code.

  Maybe this means your patch file is no longer necessary, in which case
  hooray! Just delete it!

  Otherwise, you need to generate a new patch file.

  To generate a new one, just repeat the steps you made to generate the first
  one.

  i.e. manually make the appropriate file changes, then run 

    patch-package @metamask/design-tokens

  Info:
    Patch file: patches/@metamask+design-tokens+1.4.2.patch
    Patch was made for version: 1.4.2
    Installed version: 1.5.1

---
patch-package finished with 1 error(s).
4. Create xcconfig files...

@chrisleewilcox chrisleewilcox added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed and removed QA in Progress QA has started on the feature. labels Apr 12, 2022
@blackdevelopa blackdevelopa force-pushed the patch-token-detection/network-education-modal branch from afcc75f to f866f49 Compare April 14, 2022 15:56
@chrisleewilcox chrisleewilcox added needs-qa Any New Features that needs a full manual QA prior to being added to a release. QA in Progress QA has started on the feature. and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Apr 14, 2022
@cortisiko cortisiko removed the QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed label Apr 14, 2022
@chrisleewilcox
Copy link
Contributor

The modal is not translated for other languages....
image

@chrisleewilcox chrisleewilcox added QA Passed A successful QA run through has been done QA'd but questions A QA run through has been done but you need clarification on minor issues you found and removed QA in Progress QA has started on the feature. labels Apr 14, 2022
chrisleewilcox
chrisleewilcox approved these changes Apr 14, 2022
View reviewed changes
Copy link
Contributor

@chrisleewilcox chrisleewilcox 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 to merge. Translation can be handle in a different issue.

@Cal-L Cal-L merged commit e8f18e9 into main Apr 15, 2022
@Cal-L Cal-L deleted the patch-token-detection/network-education-modal branch April 15, 2022 00:04
@github-actions github-actions bot locked and limited conversation to collaborators Apr 15, 2022
@mobularay mobularay removed the QA'd but questions A QA run through has been done but you need clarification on minor issues you found label Apr 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@chrisleewilcox chrisleewilcox chrisleewilcox approved these changes

@owencraston owencraston owencraston approved these changes

Assignees

@blackdevelopa blackdevelopa

Labels
QA Passed A successful QA run through has been done
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@blackdevelopa @owencraston @chrisleewilcox @Cal-L @cortisiko @mobularay