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

feat: removed feature flag for confirmations screen #27877

Merged
merged 18 commits into from
Oct 22, 2024
Merged

feat: removed feature flag for confirmations screen #27877

merged 18 commits into from
Oct 22, 2024

Conversation

NidhiKJha
Copy link
Member

@NidhiKJha NidhiKJha commented Oct 15, 2024 •
edited
Loading

This PR is to remove the feature flag from confirmations page

Related issues

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/3517

Manual testing steps

  1. Connect extension to a Dapp
  2. Try to switch to a non enabled network from Dapp
  3. In the confirmations screen, we should not see the network picker
  4. The Definition list component should expand and popup should not show up on click of See Details

Screenshots/Recordings

Before

Screenshot 2024-10-16 at 5 55 15 PM

After

Screenshot 2024-10-16 at 5 54 55 PM

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@NidhiKJha NidhiKJha requested review from a team as code owners October 15, 2024 19:22
@metamaskbot metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Oct 15, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [7c2250d]
Page Load Metrics (1908 ± 83 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint16132242191318087
domContentLoaded16042155187216881
load16132162190817283
domInteractive237749189
backgroundConnect882392512
firstReactRender462061014220
getState46614178
initialActions01000
loadScripts11781703143315675
setupStore1176332512
uiStartup18082509214020398
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 0 Bytes (0.00%)
  • ui: -730 Bytes (-0.01%)
  • common: -1.38 KiB (-0.02%)

@metamaskbot
Copy link
Collaborator

Builds ready [894cf7a]
Page Load Metrics (1993 ± 139 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint29628191915480230
domContentLoaded168227581962282136
load169128281993289139
domInteractive21101552311
backgroundConnect8108343517
firstReactRender692721204622
getState5158173416
initialActions00000
loadScripts117821211450253122
setupStore1176362612
uiStartup190231992290382183
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 0 Bytes (0.00%)
  • ui: -730 Bytes (-0.01%)
  • common: -1.38 KiB (-0.02%)

@NidhiKJha NidhiKJha added the needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review. label Oct 16, 2024
@sahar-fehri
Copy link
Contributor

sahar-fehri commented Oct 16, 2024 •
edited
Loading

Thanks @NidhiKJha ! Looks good; i have tested this locally; when switching to Base i do see a warning at the top which does not appear in the screenshot just wanted to make sure its expected
Screenshot 2024-10-16 at 19 32 30

@sahar-fehri
Copy link
Contributor

Did not have latest changes; tested locally again and i dnt see the warning anymore

sahar-fehri
sahar-fehri previously approved these changes Oct 16, 2024
View reviewed changes
@sahar-fehri
Copy link
Contributor

LGTM! thnx 🙏

sahar-fehri
sahar-fehri previously approved these changes Oct 16, 2024
View reviewed changes
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Oct 16, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
80.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@metamaskbot
Copy link
Collaborator

Builds ready [15b16a0]
Page Load Metrics (2941 ± 164 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint428342520721204578
domContentLoaded223836072913348167
load232236302941342164
domInteractive5512592199
backgroundConnect11128442914
firstReactRender803831787235
getState1083322010
initialActions01000
loadScripts169927372241296142
setupStore19122512512
uiStartup258940003385401192
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 0 Bytes (0.00%)
  • ui: -685 Bytes (-0.01%)
  • common: -2.09 KiB (-0.03%)

@metamaskbot
Copy link
Collaborator

Builds ready [a06d36c]
Page Load Metrics (1884 ± 98 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint22926111810410197
domContentLoaded17042377183515072
load17112653188420498
domInteractive25108512311
backgroundConnect9270555929
firstReactRender471961033818
getState4119233316
initialActions00000
loadScripts12291741136110952
setupStore10165373919
uiStartup187235002164374179
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 0 Bytes (0.00%)
  • ui: -687 Bytes (-0.01%)
  • common: -2.09 KiB (-0.03%)

@metamaskbot
Copy link
Collaborator

Builds ready [fd377f5]
Page Load Metrics (1952 ± 140 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint165328161950276133
domContentLoaded160026611905243117
load161028791952291140
domInteractive24103492311
backgroundConnect8220445928
firstReactRender543021145426
getState592242612
initialActions01000
loadScripts11621999140619292
setupStore1296312512
uiStartup180234582218382183
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 0 Bytes (0.00%)
  • ui: -687 Bytes (-0.01%)
  • common: -2.09 KiB (-0.03%)

@metamaskbot
Copy link
Collaborator

Builds ready [6091121]
Page Load Metrics (1779 ± 57 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint24920151624467224
domContentLoaded15621953175110751
load15702017177911857
domInteractive2295442010
backgroundConnect782302211
firstReactRender48212954120
getState44512115
initialActions00000
loadScripts1155143312838641
setupStore10100373014
uiStartup17732444199518288
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 0 Bytes (0.00%)
  • ui: -687 Bytes (-0.01%)
  • common: -2.09 KiB (-0.03%)

NidhiKJha
NidhiKJha commented Oct 21, 2024
View reviewed changes
ui/pages/confirmations/confirmation/templates/add-ethereum-chain.js
Comment on lines -268 to -271
element: 'Chip',
key: 'network-chip',
props: {
label: pendingApproval.requestData.chainName,
Copy link
Member Author

Choose a reason for hiding this comment

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

Chip was removed because we don't want to show network chip or network picker anymore as per figma design .

ui/pages/confirmations/confirmation/templates/add-ethereum-chain.js
],
},
multichainFlag && {
element: 'BannerAlert',
Copy link
Member Author

Choose a reason for hiding this comment

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

We have callouts to show different alerts for confirmations. So, to show a banner for just one case wasn't working fine. Hence we decided to remove it. cc @mmragkandala

ui/pages/confirmations/confirmation/confirmation.js Outdated Show resolved Hide resolved
@metamaskbot
Copy link
Collaborator

Builds ready [556ea30]
Page Load Metrics (1855 ± 98 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint160427071870230111
domContentLoaded15902450181718488
load16042556185520598
domInteractive147448157
backgroundConnect13123433215
firstReactRender492021024120
getState5173203718
initialActions01000
loadScripts11681808134214168
setupStore1284342311
uiStartup177234172113350168
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 0 Bytes (0.00%)
  • ui: -682 Bytes (-0.01%)
  • common: -2.09 KiB (-0.03%)

@NidhiKJha NidhiKJha added this pull request to the merge queue Oct 22, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 22, 2024
@NidhiKJha NidhiKJha added this pull request to the merge queue Oct 22, 2024
Merged via the queue into develop with commit 088983a Oct 22, 2024
76 checks passed
@NidhiKJha NidhiKJha deleted the feature-flg-confirmation branch October 22, 2024 14:07
@github-actions github-actions bot locked and limited conversation to collaborators Oct 22, 2024
@metamaskbot metamaskbot added the release-12.7.0 Issue or pull request that will be included in release 12.7.0 label Oct 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sahar-fehri sahar-fehri sahar-fehri approved these changes

@pedronfigueiredo pedronfigueiredo pedronfigueiredo approved these changes

Assignees
No one assigned
Labels
INVALID-PR-TEMPLATE PR's body doesn't match template needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review. release-12.7.0 Issue or pull request that will be included in release 12.7.0 team-wallet-ux
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@NidhiKJha @metamaskbot @sahar-fehri @pedronfigueiredo