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

fix: add favicon fetching hook #6951

Merged
merged 50 commits into from
Oct 25, 2023
Merged

fix: add favicon fetching hook #6951

merged 50 commits into from
Oct 25, 2023

Conversation

NicolasMassart
Copy link
Contributor

@NicolasMassart NicolasMassart commented Aug 2, 2023

Description

  • Replace Faviconkit url with a hook that returns favicon url
  • make it work also for SVG files and handle fallback
  • create a favicon utils file for fetching in HTML and dealing with origin and urls
  • create wrapper to use hook in class components
  • update reducer to store favicon url in the state as a cache (max 100)
  • create new unit tests and update impacted existing ones
  • also added a list of files to ignore by the yarn format tool so that we can run it in our branches without having tons of unexpected changes on unrelated files

Issue

fixes https://github.com/MetaMask/mobile-planning/issues/1161

E2E tests

🟢 Pipeline build #9783

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

Test Scenarios

Scenario: Website favicon should appear next to an autocompleted result when searching for websites with a favicon

Case: This is a normal use case of autocompleting a dapp that exists in the dapps list

  • Given I am on the browser tab
  • And I have the default Metamask home page open (home.metamask.io)
  • When I type open in the search bar
  • Then a image favicon should appear to the left of the autocompleted option opensea.io
  • OpenSea logo

Scenario: Fallback letter should appear next to an autocompleted result when searching for websites without a favicon

Case: this is an error case where the dapp that exists in the dapps list either doesn't have a favicon or favicon is not available.

  • Given I am on the browser tab
  • And I have the default Metamask home page open (home.metamask.io)
  • When I type metamask in the search bar
  • Then a M favicon should appear to the left of the autocompleted option Metamask Ether Faucet (Ropsten) as the site doesn't exist anymore.
  • image

Scenario: Website favicon should appear on the browser tabs list

Case: This is a normal use case for websites that have a favicon

  • Given I am on the browser tab
  • And I have the default Metamask home page open (home.metamask.io)
  • When I type app.uniswap.org in the search bar and validate
  • Then the Uniswap website opens
  • image
  • When I touch the bottom bar tab action image
  • Then the tabs list appears showing one tab for Uniswap with the website favicon
  • image

Scenario: Website favicon should appear in connect and permission modals

Case: This is a normal use case for websites that have a favicon

  • Given I am on the browser tab
  • And I have the default Metamask home page open (home.metamask.io)
  • And the test dapp website is loaded
  • image
  • When I touch "connect" in "Basic Actions" on the dapp
  • Then a connect modal appears showing the dapp favicon and the URL
  • image
  • When I touch "Connect"
  • Then the modal hides
  • And dapp "Connect" button is disabled and top right account button image appears
  • When I touch top right account button
  • Then the connected accounts modal appears with the favicon and the domain name
  • image
  • When I touch permission link
  • Then Permission modal appears with favicon and url of the dapp
  • image

Scenario: Website favicon should appear in transaction modal

Case: This is a normal use case for websites that have a favicon

  • Given I am on the test dapp
  • And I connected my account
  • When I touch the "send legacy transaction" button
  • Then the transaction modal displays the dapp favicon
  • image

Scenario: Website favicon should appear in signing modal

Case: This is a normal use case for websites that have a favicon

  • Given I am on the test dapp
  • And I connected my account
  • When I touch the "Sign" button in Personal sign section
  • Then the signing modal displays the dapp favicon
  • image

Scenario: Website favicon should appear in favourites on home page

NOTE: this scenario can not be successful until the home.metamask.io page is also updated

Case: This is a normal use case for websites that have a favicon

  • Given I added a website with a favicon to my favourites
  • Screenshot 2023-09-26 at 17 19 29image
  • When I touch the home button in the browser bar
  • Then the home page should display my favourited site in the list of favourites
  • And the favicon should be the same as the browser tabs list one

Scenario: Website favicon should appear in connect modal when deeplinking from WalletConnect

Case: This is a normal use case for websites that have a favicon

  • Given I am on a dapp (ie. app.uniswap.org) on my native mobile browser
  • And I connected my account with the WalletConnect feature
  • And I touch the "connect with Metamasks" button
  • And the MM app opens with the wallet connect modal for this dapp
  • Then the wallet connect modal on MM app displays the dapp favicon
screen-20231010-155602.mp4

Scenario: Website favicon should appear in connect modal when scanning QRCode from SDK enabled dapp

Case: This is a normal use case for websites that use MM SDK

  • Given I am on a SDK enabled dapp (ie. SDK test dapp) on desktop browser
  • And I connected my account with the connect button
  • image (this QRCode screenshot can be directly used for testing)
  • And I scan the QRcode with Metamask Mobile app
  • And the MM app opens with the SDK connect modal for this dapp
  • Then the SDK connect modal on MM app displays a QRCode Favicon
screen-20231011-142702.mp4

@github-actions
Copy link
Contributor

github-actions bot commented Aug 2, 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.

@NicolasMassart NicolasMassart changed the title add favicon fetching hook fix: add favicon fetching hook Aug 2, 2023
@socket-security
Copy link

socket-security bot commented Aug 3, 2023

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
@xmldom/xmldom 0.8.10 None +0 182 kB karfau

@hesterbruikman
Copy link
Contributor

PR for home.metamask.io

@NicolasMassart @cortisiko it appears the 188 cannot be tested unless merged. I'll ask to hold of to next sprint so that both PR's can be merged and tested in parallel. Please let me know if that doesn't make sense.

@NicolasMassart
Copy link
Contributor Author

NicolasMassart commented Oct 3, 2023

As discussed with @cortisiko;

  • add the state migration for people updating app who already have bookmarked dapps.
  • handle the deeplinking use case (from dapp -> walletconnnect -> connect sheet should display the right favicon)

Cal-L
Cal-L previously approved these changes Oct 10, 2023
Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

Code LGTM.

@Cal-L Cal-L added the DO-NOT-MERGE Pull requests that should not be merged label Oct 17, 2023
@Cal-L
Copy link
Contributor

Cal-L commented Oct 17, 2023

Added Do not merge label. Should be part of 7.11.0

Copy link
Member

@cortisiko cortisiko left a comment

Choose a reason for hiding this comment

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

✅ ✅ ✅ ✅ ✅ ✅

@cortisiko cortisiko added the QA Passed A successful QA run through has been done label Oct 24, 2023
@sonarcloud
Copy link

sonarcloud bot commented Oct 25, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

85.7% 85.7% Coverage
0.3% 0.3% Duplication

@tommasini tommasini removed blocked DO-NOT-MERGE Pull requests that should not be merged labels Oct 25, 2023
@tommasini tommasini merged commit 7f46d2b into main Oct 25, 2023
26 of 27 checks passed
@tommasini tommasini deleted the fix/1161_favicon_download branch October 25, 2023 20:12
@github-actions github-actions bot locked and limited conversation to collaborators Oct 25, 2023
@metamaskbot metamaskbot added the release-7.11.0 Issue or pull request that will be included in release 7.11.0 label Oct 25, 2023
@gauthierpetetin gauthierpetetin added team-mobile-platform team-mobile-ux DEPRECATED: please use "team-wallet-ux" label instead labels Feb 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
QA Passed A successful QA run through has been done release-7.11.0 Issue or pull request that will be included in release 7.11.0 team-mobile-platform team-mobile-ux DEPRECATED: please use "team-wallet-ux" label instead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants