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

Improvement/protect funds everywhere #1514

Merged
merged 11 commits into from
Jun 1, 2020

Conversation

estebanmino
Copy link
Contributor

@estebanmino estebanmino commented Apr 27, 2020

Description

Added the sticky bar to start account backup process everywhere but these views because it blocked the buttons.

https://github.com/MetaMask/metamask-mobile/pull/1514/files#diff-3df4426e7ab346d10de9af4518c13e37R55

Video https://recordit.co/ZEattFiNav

Checklist

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

Issue

Resolves #???

@estebanmino estebanmino added the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Apr 27, 2020
Copy link
Member

@andrepimenta andrepimenta 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 me! 👍

@estebanmino estebanmino requested a review from a team as a code owner May 27, 2020 17:46
Copy link
Contributor

@ibrahimtaveras00 ibrahimtaveras00 left a comment

Choose a reason for hiding this comment

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

Overall looks good; just saw one issue:

On Android upon confirming a txn, the warning banner hides on top of the txn notification
seen here = https://recordit.co/GMJqjKeDTn

Screen Shot 2020-05-28 at 8 33 39 PM

Another thing I noticed, not really a bug since it behaves as the currently implemented flow; when skipping the backup process it automatically directs you back to the browser.

seen here = https://recordit.co/G892w6m6Qi

Do we want to follow this format or keep the user on whichever view they were previously on before tapping on the warning banner @omnat @estebanmino @cjeria?

FYI, doing the secure your wallet flow does correctly keep you on whichever view the user was last on.

@ibrahimtaveras00 ibrahimtaveras00 added QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels May 29, 2020
@cjeria
Copy link

cjeria commented May 29, 2020

The backup recovery phrase notification behavior is changing soon (before v1 launch?) with a gradual nudging to back up, however, I presume this situation is still an edge-case that will occur. So to avoid alert notifs overlapping, we could use this special sticky alert banner under the top app bar which works in both app views. WDYT? @omnat

image

Do we want to follow this format or keep the user on whichever view they were previously on before tapping on the warning banner?

I agree let's take the user back to the screen they were previously coming from.

@omnat
Copy link
Contributor

omnat commented May 29, 2020

Yeah makes sense to send the user back to the screen they were on before tapping the backup prompt.

Overlapping - agree, would be good to show the backup prompt after user closes out the tx notification (or when tx notification clears out). Alternatively stack it on top of tx notif so both are visible and drop it down after tx notification clears. Your call @cjeria

Copy link
Contributor

@ibrahimtaveras00 ibrahimtaveras00 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 on both OS's, QA Passed 👍

@estebanmino estebanmino merged commit a9f235f into develop Jun 1, 2020
@estebanmino estebanmino deleted the improvement/protect-funds-everywhere branch June 1, 2020 19:38
@cjeria
Copy link

cjeria commented Jun 1, 2020

I agree with Omna's first suggestion. Let's overlap the TX notif above the backup prompt for now.

rickycodes pushed a commit that referenced this pull request Jan 31, 2022
* new component

* flags

* blocked list

* snaps

* elevation

* snaps

* fix

Co-authored-by: Ibrahim Taveras <ibrahimtaveras00@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next release QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants