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

[MMI] Added code fences in whats new popup #19581

Merged

Conversation

albertolive
Copy link
Contributor

@albertolive albertolive commented Jun 13, 2023

Explanation

Adding the MMI portfolio notifications without disrupting much of the existing MM logic while at the same time having it prepared to add new MMI-specific notifications in the future. Added code fences to three existing files:

  1. metamask-controller: We added our own notifications
  2. whats-new-popup: We added new logic just for MMI
  3. home-component: Added needed MMI prop

https://consensyssoftware.atlassian.net/browse/MMI-2503

Pre-merge author checklist

  • I've clearly explained:
    • What problem this PR is solving
    • How this problem was solved

Pre-merge reviewer checklist

  • Manual testing (e.g. pull and build branch, run in browser, test code being changed)
  • PR is linked to the appropriate GitHub issue

@albertolive albertolive requested a review from a team as a code owner June 13, 2023 15:00
@albertolive albertolive requested a review from danjm June 13, 2023 15:00
@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.

zone-live
zone-live previously approved these changes Jun 14, 2023
@metamaskbot
Copy link
Collaborator

Builds ready [b6f5462]
Page Load Metrics (1836 ± 89 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1042601543517
domContentLoaded15842214182319091
load15842214183618589
domInteractive15842214182319091
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 8 bytes
  • ui: -103 bytes
  • common: 0 bytes

@codecov
Copy link

codecov bot commented Jun 14, 2023

Codecov Report

Merging #19581 (67a211e) into develop (ec4c405) will decrease coverage by 0.02%.
The diff coverage is 38.89%.

@@             Coverage Diff             @@
##           develop   #19581      +/-   ##
===========================================
- Coverage    69.85%   69.83%   -0.02%     
===========================================
  Files          980      981       +1     
  Lines        36909    36936      +27     
  Branches      9908     9911       +3     
===========================================
+ Hits         25782    25794      +12     
- Misses       11127    11142      +15     
Impacted Files Coverage Δ
.../components/app/whats-new-popup/whats-new-popup.js 21.09% <20.00%> (-1.13%) ⬇️
app/scripts/metamask-controller.js 65.85% <50.00%> (+0.09%) ⬆️
ui/pages/home/home.component.js 44.28% <50.00%> (-0.22%) ⬇️
shared/notifications/institutional/index.js 66.67% <66.67%> (ø)

…mm-codebase

# Conflicts:
#	ui/components/app/whats-new-popup/whats-new-popup.js
NidhiKJha
NidhiKJha previously approved these changes Jun 23, 2023
@albertolive
Copy link
Contributor Author

@metamaskbot update-policies

@metamaskbot
Copy link
Collaborator

Policies updated

@metamaskbot
Copy link
Collaborator

Builds ready [e6788ee]
Page Load Metrics (1714 ± 65 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1152351493215
domContentLoaded15252042171413665
load15252043171413665
domInteractive15252042171413665
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 11 bytes
  • ui: -104 bytes
  • common: 60 bytes


///: BEGIN:ONLY_INCLUDE_IN(build-mmi)
renderNotification = renderFirstNotification;
///: END:ONLY_INCLUDE_IN
Copy link
Contributor

Choose a reason for hiding this comment

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

So... this is an interesting optimization, but I don't think it would pass typescript safety. renderFirstNotification and renderSubsequentNotification have different arguments, so even though the additional parameters for renderFirstNotification are on the end of the function parameters it would fail to compile in typescript due to extra params existing. In order to make this work you should probably change both renderFirst and renderSubsequent take an options bag ({ notification, idRefMap }) instead of positional parameters. When you add jsdoc types to them, you can define a new type using @typedef for the options and pass that type as the type for the first parameter to each function. Even if you don't reference the 4 additional properties on renderFirst in renderSubsequent it would still type check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @brad-decker, I fixed this issue. Please verify that this is what you asked for.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup!

@albertolive
Copy link
Contributor Author

@metamaskbot update-policies

@metamaskbot
Copy link
Collaborator

No policy changes

@albertolive albertolive merged commit 1e56fdb into develop Jun 27, 2023
54 of 56 checks passed
@albertolive albertolive deleted the feature/MMI-3123-add-mmi-notification-to-mm-codebase branch June 27, 2023 06:30
@github-actions github-actions bot locked and limited conversation to collaborators Jun 27, 2023
@metamaskbot
Copy link
Collaborator

Builds ready [67a211e]
Page Load Metrics (1597 ± 39 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint114169140188
domContentLoaded1439176815978239
load1439176815978239
domInteractive1439176715978239
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 11 bytes
  • ui: 35 bytes
  • common: 60 bytes

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants