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

refactor(1702-4): ipfs gateway settings and display nft media settings components #11381

Merged
merged 20 commits into from
Oct 4, 2024
Merged

refactor(1702-4): ipfs gateway settings and display nft media settings components #11381

merged 20 commits into from
Oct 4, 2024

Conversation

vinnyhoward
Copy link
Contributor

@vinnyhoward vinnyhoward commented Sep 23, 2024 •
edited
Loading

Description

This PR is part of a series that breaks down the original large PR #11127 into smaller, more manageable chunks.

The IPFSGateway functionality and DisplayNFtMedia functionality are valuable beyond its original implementation. By converting it into a reusable component, we can:

  1. Improve code maintainability
  2. Reduce duplication
  3. Enhance consistency across the application and ease of use

Changes

  • Extracted IPFSGateway and DisplayNFTMedia logic from its original location of SecuritySettings into separate component files
  • Included new tests
  • Updated the original screen to use the new components
  • Removed related tests/constants within SecuritySettings and moved them into the new component folder

Impact

This refactored component will be used in:

  • Its current original screen of SecuritySettings
  • New screens coming from the enhanced onboarding settings #1702 that is currently under progress

Related issues

Feature: #1702

Manual testing steps

Display NFT Media Settings

  1. On wallet home screen click on the NFT tab and make sure your NFT media (images/etc) are showing
  2. Goto settings under Security and Privacy and goto Display NFT Media and toggle off
  3. Go back to the wallet home screen under the NFT tab and you should see the NFT media not load.
  4. Repeat steps 1-3 to see your NFT media render and not render

IPFS Gateway Settings

  1. Goto settings
  2. Click on IPFS dropdown and choose your preferred IPFS
  3. Toggle off your IPFS gateway and your dropdown should disappear

Screenshots/Recordings

Display NFT Media Settings

Toggle Off Toggle On
display_nft_media_toggle_off display_nft_media_toggle_on

IPFS Gateway Settings

Selecting new IPFS gateways Toggle Off
ipfs_gateway toggle_off_ipfs_gateway

Before

NA

After

NA

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.

@github-actions 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.

@vinnyhoward vinnyhoward added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) Run Smoke E2E Triggers smoke e2e on Bitrise labels Sep 23, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Sep 23, 2024 •
edited by metamaskbot
Loading

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: c3dbab8
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/79f66bf3-a3e0-461f-b007-7380fe85b8b0

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

  • Check the documentation if you have any doubts on how to understand the failure on bitrise

@vinnyhoward vinnyhoward marked this pull request as ready for review September 23, 2024 18:20
@vinnyhoward vinnyhoward requested a review from a team as a code owner September 23, 2024 18:20
@vinnyhoward vinnyhoward changed the title feat(1702-4): ipfs gateway settings and display nft media settings components refactor(1702-4): ipfs gateway settings and display nft media settings components Sep 23, 2024
@vinnyhoward vinnyhoward added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Sep 24, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Sep 24, 2024 •
edited by metamaskbot
Loading

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: fbe7f35
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/52186e88-f3a9-484c-a16a-dd93e4088c23

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@vinnyhoward
Merge branch 'main' of github.com:MetaMask/metamask-mobile into feat-…
6442b87 
…1702-4-refactor-ipfs-and-displaynftmedia-to-reusable-components
@vinnyhoward
Merge branch 'feat-1702-4-refactor-ipfs-and-displaynftmedia-to-reusab…
cf08687 
…le-components' of github.com:MetaMask/metamask-mobile into feat-1702-4-refactor-ipfs-and-displaynftmedia-to-reusable-components
@vinnyhoward vinnyhoward added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Sep 24, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Sep 24, 2024 •
edited by metamaskbot
Loading

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: cf08687
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/214d5fce-387e-4311-bf1d-99865a6eacb1

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@vinnyhoward vinnyhoward added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Sep 25, 2024
@vinnyhoward vinnyhoward added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Sep 26, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Sep 26, 2024 •
edited by metamaskbot
Loading

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 233295e
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/78956b93-7172-4ef8-86a3-3686748df2f4

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

  • Check the documentation if you have any doubts on how to understand the failure on bitrise

brianacnguyen
brianacnguyen previously requested changes Sep 27, 2024
View reviewed changes
Copy link
Contributor

@brianacnguyen brianacnguyen left a comment

Choose a reason for hiding this comment

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

a few minor comments + adding stories

app/components/Views/Settings/DisplayNFTMediaSettings/index.tsx Outdated Show resolved Hide resolved
app/components/Views/Settings/IPFSGatewaySettings/index.types.ts Show resolved Hide resolved
@vinnyhoward vinnyhoward added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Sep 27, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 3, 2024

https://bitrise.io/ Bitrise

🔄🔄🔄 pr_smoke_e2e_pipeline started on Bitrise...🔄🔄🔄

Commit hash: 8468534
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/4191f862-a5cc-4ebb-a54b-ef01e308be5b

Note

  • This comment will auto-update when build completes
  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

andreahaku
andreahaku previously approved these changes Oct 4, 2024
View reviewed changes
Copy link
Contributor

@andreahaku andreahaku left a comment

Choose a reason for hiding this comment

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

LGTM

@EtherWizard33 EtherWizard33 self-requested a review October 4, 2024 13:47
EtherWizard33
EtherWizard33 previously approved these changes Oct 4, 2024
View reviewed changes
@vinnyhoward
Merge branch 'main' of github.com:MetaMask/metamask-mobile into feat-…
eba1e94 
…1702-4-refactor-ipfs-and-displaynftmedia-to-reusable-components
@vinnyhoward
Merge branch 'feat-1702-4-refactor-ipfs-and-displaynftmedia-to-reusab…
23f274f 
…le-components' of github.com:MetaMask/metamask-mobile into feat-1702-4-refactor-ipfs-and-displaynftmedia-to-reusable-components
@vinnyhoward
Merge branch 'main' of github.com:MetaMask/metamask-mobile into feat-…
b693765 
…1702-4-refactor-ipfs-and-displaynftmedia-to-reusable-components
@vinnyhoward vinnyhoward dismissed stale reviews from EtherWizard33 and andreahaku via b693765 October 4, 2024 15:35
@vinnyhoward vinnyhoward added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Oct 4, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 4, 2024 •
edited by metamaskbot
Loading

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: b693765
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/2ada1054-5387-4ea2-bbe7-7aba2c6506ef

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

andreahaku
andreahaku approved these changes Oct 4, 2024
View reviewed changes
Copy link
Contributor

@andreahaku andreahaku left a comment

Choose a reason for hiding this comment

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

LGTM

@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Oct 4, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarCloud

@vinnyhoward vinnyhoward enabled auto-merge October 4, 2024 15:58
@vinnyhoward vinnyhoward dismissed brianacnguyen’s stale review October 4, 2024 16:07

Opting out of stories for now in favor of getting this out quickly to create more time for Q4 features

@vinnyhoward vinnyhoward added this pull request to the merge queue Oct 4, 2024
@vinnyhoward vinnyhoward removed this pull request from the merge queue due to a manual request Oct 4, 2024
@vinnyhoward vinnyhoward added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Oct 4, 2024
@vinnyhoward vinnyhoward added this pull request to the merge queue Oct 4, 2024
Merged via the queue into main with commit f721b8d Oct 4, 2024
55 checks passed
@vinnyhoward vinnyhoward deleted the feat-1702-4-refactor-ipfs-and-displaynftmedia-to-reusable-components branch October 4, 2024 17:44
@github-actions github-actions bot locked and limited conversation to collaborators Oct 4, 2024
@github-actions github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Oct 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gambinish gambinish gambinish left review comments

@andreahaku andreahaku andreahaku approved these changes

@EtherWizard33 EtherWizard33 EtherWizard33 left review comments

@brianacnguyen brianacnguyen Awaiting requested review from brianacnguyen

Assignees
No one assigned
Labels
release-7.34.0 Run Smoke E2E Triggers smoke e2e on Bitrise team-wallet-ux
Projects
PR review queue
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@vinnyhoward @andreahaku @brianacnguyen @gambinish @EtherWizard33 @gauthierpetetin