Skip to content

[Bug]: Multiple overlapping spinners while loading redesigned confirmations #27972

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
Closed
bschorchit opened this issue Oct 18, 2024 · 0 comments · Fixed by #28301
Closed

[Bug]: Multiple overlapping spinners while loading redesigned confirmations #27972

bschorchit opened this issue Oct 18, 2024 · 0 comments · Fixed by #28301
Assignees
@pedronfigueiredo
Labels
regression-main Regression bug that was found on main branch, but not yet present in production release-12.8.0 Issue or pull request that will be included in release 12.8.0 Sev3-low Low severity; minimal to no impact upon users team-confirmations Push issues to confirmations team type-bug Something isn't working

Comments

@bschorchit
Copy link

Describe the bug

An user can see multiple while the confirmation is loading. Could we reduce it? E.g. combine the two that show up on the top of the page and remove the big and different one that shows in the middle of the confirmation?

Expected behavior

No response

Screenshots/Recordings

Screenshot 2024-10-18 at 18 11 08

Steps to reproduce

  1. Go to test dapp
  2. Click on malicious erc20 transfer to see the 2 spinners on top and the different one in the middle
  3. Click on malicious erc20 approve to see the different spinner in the middle

Error messages or log output

No response

Detection stage

On the development branch

Version

12.8

Build type

None

Browser

Brave

Operating system

MacOS

Hardware wallet

No response

Additional context

No response

Severity

No response
@bschorchit bschorchit added type-bug Something isn't working needs-design Needs design support. Sev3-low Low severity; minimal to no impact upon users team-confirmations Push issues to confirmations team labels Oct 18, 2024
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by team Oct 18, 2024
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by severity Oct 18, 2024
@metamaskbot metamaskbot added the regression-main Regression bug that was found on main branch, but not yet present in production label Oct 18, 2024
@pedronfigueiredo pedronfigueiredo self-assigned this Nov 5, 2024
@pedronfigueiredo pedronfigueiredo mentioned this issue Nov 5, 2024
Merged
7 tasks
github-merge-queue bot pushed a commit that referenced this issue Nov 14, 2024
@pedronfigueiredo
fix: Remove multiple overlapping spinners (#28301)
b7c3f83 
<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

We were previously using a UI blocking, whole page spinner while loading
the redesigned confirmations. This overlapped with the individual
component spinners we use on some components that decode transaction
data. That second pattern is preferable because the spinner is contained
and doesn't block the user from taking action.

The global spinner comes from `routes.component`. The first part of this
fix is to bypass it in that file for redesigned confirmations.

The second aspect of this fix is to not condense two component spinners
into one. Since the transaction flow section always loads before the
send heading component, we can omit the spinner for that first
component.

Finally, this PR fixes the loading behaviour on the send heading
component so that if the decimals amount coming from `useAssetDetails`
hasn't been received yet, the heading is not shown, preventing
undesireable flickering in the hero value.



<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/28301?quickstart=1)

## **Related issues**

Fixes: #27972

## **Manual testing steps**

1. Go to test dapp
2. Click on malicious erc20 transfer to see the 2 spinners on top and
the different one in the middle
3. Click on malicious erc20 approve to see the different spinner in the
middle

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

<img width="362" alt="Screenshot 2024-10-18 at 18 11 08"
src="https://github.com/user-attachments/assets/9fbc5c1e-229c-4dfa-8231-4957a1ecf713">

### **After**

<!-- [screenshots/recordings] -->


https://github.com/user-attachments/assets/2f5dd8c2-3b9b-405d-b9b5-0eb8bf793fcb


## **Pre-merge author checklist**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **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.
@github-project-automation github-project-automation bot moved this from To be fixed to Fixed in Bugs by team Nov 14, 2024
@github-actions github-actions bot removed the needs-design Needs design support. label Nov 14, 2024
@metamaskbot metamaskbot added the release-12.8.0 Issue or pull request that will be included in release 12.8.0 label Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@pedronfigueiredo pedronfigueiredo

Labels
regression-main Regression bug that was found on main branch, but not yet present in production release-12.8.0 Issue or pull request that will be included in release 12.8.0 Sev3-low Low severity; minimal to no impact upon users team-confirmations Push issues to confirmations team type-bug Something isn't working
Projects
Bugs by severity
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: Remove multiple overlapping spinners MetaMask/metamask-extension
3 participants
@pedronfigueiredo @bschorchit @metamaskbot