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

Added toaster for removed NFTs #17297

Merged
merged 5 commits into from
Jan 23, 2023
Merged

Added toaster for removed NFTs #17297

merged 5 commits into from
Jan 23, 2023

Conversation

NidhiKJha
Copy link
Member

@NidhiKJha NidhiKJha commented Jan 19, 2023 •
edited
Loading

Added the toaster function with an "NFT removed" message as we have for "Successfully Added Collectibles".

Fixes #17140

After

Screen.Recording.2023-01-19.at.11.43.09.PM.mov

Pre-merge author checklist

  • I've clearly explained:
    • What problem this PR is solving
    • How this problem was solved
    • How reviewers can test my changes

Pre-merge reviewer checklist

  • Manual testing (e.g. pull and build branch, run in the browser, test code being changed)
  • PR is linked to the appropriate GitHub issue
  • IF this PR fixes a bug in the release milestone, add this PR to the release milestone

@NidhiKJha NidhiKJha requested a review from a team as a code owner January 19, 2023 17:40
@NidhiKJha NidhiKJha added area-NFTs team-extension-ux DEPRECATED: please use "team-wallet-ux" label instead labels Jan 19, 2023
@NidhiKJha NidhiKJha requested a review from darkwing January 19, 2023 18:04
@NidhiKJha NidhiKJha self-assigned this Jan 19, 2023
darkwing
darkwing requested changes Jan 19, 2023
View reviewed changes
Copy link
Contributor

@darkwing darkwing left a comment

Choose a reason for hiding this comment

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

This is very close, just needs a few changes!

ui/pages/home/home.component.js Outdated

{removeCollectibleMessage === 'success' ? (
<ActionableMessage
type="success"
Copy link
Contributor

Choose a reason for hiding this comment

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

We want this to be type="danger" so that it's red.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated

Screenshot 2023-01-20 at 11 25 05 AM

ui/pages/settings/security-tab/__snapshots__/security-tab.test.js.snap Outdated Show resolved Hide resolved
@metamaskbot
Copy link
Collaborator

Builds ready [4821922]
Page Load Metrics (1379 ± 126 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint103181127199
domContentLoaded96718251340266128
load96718251379263126
domInteractive96718251340266128
Bundle size diffs
  • background: 0 bytes
  • ui: 1265 bytes
  • common: 0 bytes

highlights:

storybook

@NidhiKJha NidhiKJha requested a review from darkwing January 20, 2023 05:56
@metamaskbot
Copy link
Collaborator

Builds ready [00149f5]
Page Load Metrics (1401 ± 135 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1032421363014
domContentLoaded99318891376281135
load101519171401281135
domInteractive99318891376281135

highlights:

storybook

@codecov-commenter
Copy link

Codecov Report

Merging #17297 (00149f5) into develop (cc518b9) will decrease coverage by 0.02%.
The diff coverage is 7.69%.

@@             Coverage Diff             @@
##           develop   #17297      +/-   ##
===========================================
- Coverage    59.80%   59.79%   -0.02%     
===========================================
  Files          937      937              
  Lines        36141    36154      +13     
  Branches      9284     9287       +3     
===========================================
+ Hits         21614    21615       +1     
- Misses       14527    14539      +12
Impacted Files Coverage Δ
...nts/app/collectible-details/collectible-details.js 0.00% <0.00%> (ø)
ui/ducks/app/app.js 52.17% <0.00%> (-0.77%) ⬇️
ui/pages/home/home.component.js 0.00% <0.00%> (ø)
ui/pages/home/home.container.js 0.00% <0.00%> (ø)
ui/selectors/selectors.js 65.19% <0.00%> (-0.23%) ⬇️
ui/store/actions.js 39.08% <0.00%> (-0.04%) ⬇️
ui/store/actionConstants.js 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@NidhiKJha NidhiKJha requested a review from DDDDDanica January 20, 2023 17:10
@NidhiKJha NidhiKJha merged commit 180eabe into develop Jan 23, 2023
@NidhiKJha NidhiKJha deleted the fix-17140 branch January 23, 2023 11:53
@github-actions github-actions bot locked and limited conversation to collaborators Jan 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@DDDDDanica DDDDDanica DDDDDanica approved these changes

@darkwing darkwing darkwing approved these changes

@georgewrmarshall georgewrmarshall Awaiting requested review from georgewrmarshall georgewrmarshall is a code owner automatically assigned from MetaMask/extension-devs

Assignees

@NidhiKJha NidhiKJha

Labels
area-NFTs team-extension-ux DEPRECATED: please use "team-wallet-ux" label instead
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

NFTs: Clicking "Remove item" removes item without feedback
6 participants
@NidhiKJha @metamaskbot @codecov-commenter @darkwing @DDDDDanica @pedronfigueiredo