-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Whats new popup #10583
Whats new popup #10583
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. |
This requires a migration to ensure that users who have already seen the swaps popup do not see the swaps notification |
Done in d84fb66 |
e2e test failures will be resolved when MetaMask/core#375 and MetaMask/core#329 are merged and a new version of controllers is released |
be756f1
to
8b6530d
Compare
There are a few conflicts to resolve it seems |
ed43231
to
572b4f9
Compare
Builds ready [b0d8463]
Page Load Metrics (660 ± 22 ms)
|
Builds ready [0c882e3]
Page Load Metrics (646 ± 23 ms)
|
Final notification images and messages (as confirmed with Jacob and Omna) were added in this commit: 63128a1 |
QA test of a couple scenarios looks like this: Peek.2021-04-27.22-15.mp4 |
Builds ready [233b70e]
Page Load Metrics (570 ± 40 ms)
|
Builds ready [e805cf2]
Page Load Metrics (615 ± 64 ms)
|
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
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.
LGTM!
Had a few more suggestions that don't need to block, about how the notification render functions could be simplified.
Builds ready [b4849b5]
Page Load Metrics (658 ± 42 ms)
|
@Gudahtt I addressed your latest comments in the latest commit. You said they were non-blocking, but I liked those suggestions and its probably easier to resolve them in this PR. |
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
Okay, final QA pass looks good: Peek.2021-04-28.11-43.mp4 |
Builds ready [190fbcb]
Page Load Metrics (723 ± 58 ms)
|
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.
LGTM!
Left one more suggestion that I thought of after seeing that video. Also I hadn't noticed how small this popup looks until now. It looks especially silly on large screens, having to squish the text into such a small area. This seems to match the designs, so maybe this is good for now, but I think it would be better to let the popup be larger, to make the best use of the space we have.
Builds ready [7b120a4]
Page Load Metrics (830 ± 64 ms)
|
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.
LGTM!
Builds ready [507263b]
Page Load Metrics (637 ± 42 ms)
|
Fixes /~https://github.com/MetaMask/MetaMask-planning/issues/23
This PR adds the "See what's new" feature to the extension. This PR requires #329 of /~https://github.com/MetaMask/controllers, and #375 which aims to update #329
Videos of the behaviour are below:
Peek.2021-03-04.02-22.mp4
Peek.2021-03-04.02-20.mp4
Peek.2021-03-03.17-55.mp4