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

Added IPFS toggle #20172

Merged
merged 25 commits into from
Aug 1, 2023
Merged

Added IPFS toggle #20172

merged 25 commits into from
Aug 1, 2023

Conversation

NidhiKJha
Copy link
Member

@NidhiKJha NidhiKJha commented Jul 25, 2023

This PR:

  1. Adds an IPFS toggle
  2. If IPFS Toggle is on, the user can input the IPFS value
  3. In case IPFS is toggled off, no NFT image will be shown
  4. Placeholder image in case, when IPFS is off, will have a show button, on click of that IPFS toggle modal pops up

Screencaps

Screen.Recording.2023-07-28.at.6.32.05.PM.mov

Manual Testing Steps

  • Turn on the IPFS toggle
  • Input field should be hidden
  • No NFT media should be shown if it's stored in ipfs
  • Toggle on, by default the default IPFS is restored, you can edit it as well

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
  • Sufficient automated test coverage has been added

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
  • IF this PR fixes a bug in the release milestone, add this PR to the release milestone

If further QA is required (e.g. new feature, complex testing steps, large refactor), add the Extension QA Board label.

In this case, a QA Engineer approval will be be required.

@NidhiKJha NidhiKJha requested a review from a team as a code owner July 25, 2023 09:12
@NidhiKJha NidhiKJha added the DO-NOT-MERGE Pull requests that should not be merged label Jul 25, 2023
@HowardBraham
Copy link
Contributor

I don't think this UI is clear as proposed. The toggle as the text is written would turn the custom gateway off. Your intention is to turn all IPFS off.

You should also be aware of (and probably review) my very related PR #19700

@NidhiKJha
Copy link
Member Author

I don't think this UI is clear as proposed. The toggle as the text is written would turn the custom gateway off. Your intention is to turn all IPFS off.

@HowardBraham this PR is just to add the toggle. For other UI improvements as per the Figma design, I am going to create a separate PR. The text custom IPFS will be replaced as well. I couldn't find another place where I need to update the IPFS value, so I think this toggle will turn all IPFS off. Right?

@NidhiKJha NidhiKJha added team-extension-client team-extension-ux DEPRECATED: please use "team-wallet-ux" label instead and removed DO-NOT-MERGE Pull requests that should not be merged labels Jul 26, 2023
@NidhiKJha NidhiKJha changed the title (WIP 👷‍♀️ ) Added IPFS toggle Added IPFS toggle Jul 26, 2023
@metamaskbot
Copy link
Collaborator

Builds ready [c7badf4]
Page Load Metrics (1715 ± 73 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1151971452613
domContentLoaded15152020171515173
load15152020171515173
domInteractive15152020171515173
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 573 Bytes (0.01%)
  • common: 0 Bytes (0.00%)

@codecov
Copy link

codecov bot commented Jul 27, 2023

Codecov Report

Merging #20172 (350e0a3) into develop (41f6627) will increase coverage by 0.64%.
Report is 28 commits behind head on develop.
The diff coverage is 32.61%.

❗ Current head 350e0a3 differs from pull request most recent head c15d839. Consider uploading reports for the commit c15d839 to get more accurate results

@@             Coverage Diff             @@
##           develop   #20172      +/-   ##
===========================================
+ Coverage    68.68%   69.31%   +0.64%     
===========================================
  Files          986      988       +2     
  Lines        37870    37323     -547     
  Branches     10138    10021     -117     
===========================================
- Hits         26008    25870     -138     
+ Misses       11862    11453     -409     
Files Changed Coverage Δ
ui/components/app/nft-details/nft-details.js 93.33% <ø> (-0.09%) ⬇️
ui/components/multichain/nft-item/nft-item.js 100.00% <ø> (ø)
ui/ducks/app/app.ts 60.48% <0.00%> (-2.02%) ⬇️
ui/pages/routes/routes.container.js 52.63% <0.00%> (-2.92%) ⬇️
ui/store/actions.ts 41.76% <0.00%> (-0.46%) ⬇️
...ponents/app/nft-default-image/toggle-ipfs-modal.js 16.67% <16.67%> (ø)
...es/settings/security-tab/security-tab.component.js 73.53% <27.27%> (-3.89%) ⬇️
...ponents/app/nft-default-image/nft-default-image.js 70.00% <50.00%> (-30.00%) ⬇️
ui/pages/routes/routes.component.js 48.95% <50.00%> (+0.01%) ⬆️
ui/components/app/nfts-items/nfts-items.js 95.16% <100.00%> (+0.08%) ⬆️
... and 1 more

... and 75 files with indirect coverage changes

@NidhiKJha NidhiKJha added the DO-NOT-MERGE Pull requests that should not be merged label Jul 27, 2023
@NidhiKJha NidhiKJha marked this pull request as draft July 27, 2023 18:01
@NidhiKJha NidhiKJha removed the DO-NOT-MERGE Pull requests that should not be merged label Jul 28, 2023
@NidhiKJha NidhiKJha marked this pull request as ready for review July 28, 2023 08:26
@metamaskbot
Copy link
Collaborator

Builds ready [4082ecb]
Page Load Metrics (1485 ± 32 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1012031232411
domContentLoaded1368165414856732
load1368165414856732
domInteractive1368165414856732
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 4.16 KiB (0.05%)
  • common: 581 Bytes (0.01%)

@SaraCheikh
Copy link

Hi @NidhiKJha some small details about content and design:

-The old copy should be updated to the new one reviewed by @coreyjanssen : MetaMask uses third-party services to show images of your NFTs stored on IPFS, display information related to ENS addresses entered in your browser's address bar, and fetch icons for different tokens. Your IP address may be exposed to these services when you’re using them.
-When the setting is on, the toggle should have a label that says: Choose your preferred IPFS gateway

Screen.Recording.2023-07-28.at.11.11.21.mov

@NidhiKJha
Copy link
Member Author

Hi @NidhiKJha some small details about content and design:

@SaraCheikh, we will update the Design and copy of the settings page along with the autodetection toggle and openSea. I have updated the Placeholder image with the one that you shared in the screencast.

@metamaskbot
Copy link
Collaborator

Builds ready [350e0a3]
Page Load Metrics (1586 ± 61 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint113181131188
domContentLoaded14321943158612861
load14321943158612861
domInteractive14321943158612861
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 3.73 KiB (0.05%)
  • common: 581 Bytes (0.01%)

ui/components/app/nfts-items/nfts-items.js Outdated Show resolved Hide resolved
ui/components/app/nfts-items/nfts-items.js Outdated Show resolved Hide resolved
ui/components/multichain/nft-item/nft-item.js Outdated Show resolved Hide resolved
@SaraCheikh
Copy link

we will update the Design and copy of the settings page along with the autodetection toggle and openSea
sounds good @NidhiKJha

@NidhiKJha NidhiKJha requested review from kumavis and a team as code owners August 1, 2023 12:59
@github-actions
Copy link
Contributor

github-actions bot commented Aug 1, 2023

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.

@NidhiKJha NidhiKJha removed request for a team and kumavis August 1, 2023 13:03
@metamaskbot
Copy link
Collaborator

Builds ready [6457684]
Page Load Metrics (1656 ± 97 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1002421503818
domContentLoaded14042035165620297
load14052035165620297
domInteractive14042035165620297
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 4.18 KiB (0.05%)
  • common: 581 Bytes (0.02%)

@NidhiKJha NidhiKJha requested a review from DDDDDanica August 1, 2023 16:11
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.

Approving with an issue being opened to create an E2E

@metamaskbot
Copy link
Collaborator

Builds ready [a5c89b6]
Page Load Metrics (1528 ± 51 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint115169135147
domContentLoaded14031886152810751
load14031886152810751
domInteractive14031886152810751
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 4.18 KiB (0.05%)
  • common: 581 Bytes (0.02%)

@NidhiKJha NidhiKJha merged commit 4c37448 into develop Aug 1, 2023
@NidhiKJha NidhiKJha deleted the fix-872-a branch August 1, 2023 18:21
@github-actions github-actions bot locked and limited conversation to collaborators Aug 1, 2023
@github-actions github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Aug 1, 2023
@metamaskbot metamaskbot added the release-10.36.0 Issue or pull request that will be included in release 10.36.0 label Aug 1, 2023
@Gudahtt Gudahtt added release-11.1.0 Issue or pull request that will be included in release 11.1.0 and removed release-10.36.0 Issue or pull request that will be included in release 10.36.0 labels Sep 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-privacy release-11.1.0 Issue or pull request that will be included in release 11.1.0 team-extension-ux DEPRECATED: please use "team-wallet-ux" label instead
Projects
None yet
Development

Successfully merging this pull request may close these issues.