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

Whats new popup #10583

Merged
merged 48 commits into from
Apr 28, 2021
Merged

Whats new popup #10583

merged 48 commits into from
Apr 28, 2021

Conversation

danjm
Copy link
Contributor

@danjm danjm commented Mar 4, 2021

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

@danjm danjm requested a review from a team as a code owner March 4, 2021 06:26
@danjm danjm requested a review from ryanml March 4, 2021 06:26
@github-actions
Copy link
Contributor

github-actions bot commented Mar 4, 2021

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.

shared/notifications/index.js Outdated Show resolved Hide resolved
shared/notifications/index.js Outdated Show resolved Hide resolved
@danjm danjm force-pushed the whats-new-popup branch from f92f532 to 7943400 Compare March 5, 2021 00:32
@danjm
Copy link
Contributor Author

danjm commented Mar 5, 2021

This requires a migration to ensure that users who have already seen the swaps popup do not see the swaps notification

@danjm
Copy link
Contributor Author

danjm commented Mar 8, 2021

This requires a migration to ensure that users who have already seen the swaps popup do not see the swaps notification

Done in d84fb66

@danjm
Copy link
Contributor Author

danjm commented Mar 8, 2021

e2e test failures will be resolved when MetaMask/core#375 and MetaMask/core#329 are merged and a new version of controllers is released

@danjm danjm force-pushed the whats-new-popup branch 2 times, most recently from be756f1 to 8b6530d Compare March 17, 2021 06:06
@Gudahtt
Copy link
Member

Gudahtt commented Mar 18, 2021

There are a few conflicts to resolve it seems

@danjm danjm force-pushed the whats-new-popup branch 3 times, most recently from ed43231 to 572b4f9 Compare March 24, 2021 03:06
@danjm danjm changed the base branch from develop to update-metamask-controllers-v6.2.0 March 24, 2021 03:06
@metamaskbot
Copy link
Collaborator

Builds ready [b0d8463]
Page Load Metrics (660 ± 22 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint45796084
domContentLoaded5857546594722
load5907556604622
domInteractive5847546594722

Base automatically changed from update-metamask-controllers-v6.2.0 to develop March 25, 2021 20:07
@danjm danjm force-pushed the whats-new-popup branch from b0d8463 to 0c882e3 Compare March 25, 2021 20:09
@metamaskbot
Copy link
Collaborator

Builds ready [0c882e3]
Page Load Metrics (646 ± 23 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint44725894
domContentLoaded5727426454823
load5737436464923
domInteractive5727426444823

app/scripts/platforms/extension.js Outdated Show resolved Hide resolved
ui/app/store/actionConstants.js Outdated Show resolved Hide resolved
ui/app/components/app/whats-new-popup/whats-new-popup.js Outdated Show resolved Hide resolved
ui/app/components/app/whats-new-popup/whats-new-popup.js Outdated Show resolved Hide resolved
ui/app/components/app/whats-new-popup/whats-new-popup.js Outdated Show resolved Hide resolved
ui/app/components/app/whats-new-popup/whats-new-popup.js Outdated Show resolved Hide resolved
ui/app/components/app/whats-new-popup/whats-new-popup.js Outdated Show resolved Hide resolved
ui/app/selectors/selectors.js Outdated Show resolved Hide resolved
ui/app/selectors/selectors.js Outdated Show resolved Hide resolved
@danjm
Copy link
Contributor Author

danjm commented Apr 28, 2021

Final notification images and messages (as confirmed with Jacob and Omna) were added in this commit: 63128a1

@danjm
Copy link
Contributor Author

danjm commented Apr 28, 2021

QA test of a couple scenarios looks like this:

Peek.2021-04-27.22-15.mp4

@metamaskbot
Copy link
Collaborator

Builds ready [233b70e]
Page Load Metrics (570 ± 40 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint43806194
domContentLoaded4036635698340
load4056645708340
domInteractive4036625688340

@metamaskbot
Copy link
Collaborator

Builds ready [e805cf2]
Page Load Metrics (615 ± 64 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint50846684
domContentLoaded38486461413465
load38686561513464
domInteractive38486461413465

Gudahtt
Gudahtt previously approved these changes Apr 28, 2021
Copy link
Member

@Gudahtt Gudahtt left a 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.

ui/app/components/app/whats-new-popup/whats-new-popup.js Outdated Show resolved Hide resolved
ui/app/components/app/whats-new-popup/whats-new-popup.js Outdated Show resolved Hide resolved
ui/app/components/app/whats-new-popup/whats-new-popup.js Outdated Show resolved Hide resolved
@metamaskbot
Copy link
Collaborator

Builds ready [b4849b5]
Page Load Metrics (658 ± 42 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint46786284
domContentLoaded4877776578742
load4897796588642
domInteractive4877776568642

@danjm
Copy link
Contributor Author

danjm commented Apr 28, 2021

@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>
@danjm
Copy link
Contributor Author

danjm commented Apr 28, 2021

Okay, final QA pass looks good:

Peek.2021-04-28.11-43.mp4

@metamaskbot
Copy link
Collaborator

Builds ready [190fbcb]
Page Load Metrics (723 ± 58 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint469467136
domContentLoaded48082172212058
load48182372312158
domInteractive48082172212058

Gudahtt
Gudahtt previously approved these changes Apr 28, 2021
Copy link
Member

@Gudahtt Gudahtt left a 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.

@metamaskbot
Copy link
Collaborator

Builds ready [7b120a4]
Page Load Metrics (830 ± 64 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint4610775157
domContentLoaded513109482613464
load522109583013364
domInteractive513109382513464

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@metamaskbot
Copy link
Collaborator

Builds ready [507263b]
Page Load Metrics (637 ± 42 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint468663105
domContentLoaded3867216368842
load3877236378842
domInteractive3867206358842

@danjm danjm merged commit b73f543 into develop Apr 28, 2021
@danjm danjm deleted the whats-new-popup branch April 28, 2021 16:51
@github-actions github-actions bot locked and limited conversation to collaborators Apr 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants