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

Improve deeplinks experience #5218

Merged
merged 16 commits into from
Feb 2, 2023
Merged

Improve deeplinks experience #5218

merged 16 commits into from
Feb 2, 2023

Conversation

jpcloureiro
Copy link
Contributor

@jpcloureiro jpcloureiro commented Nov 9, 2022 •
edited
Loading

Development & PR Process

  1. Follow MetaMask Mobile Coding Standards
  2. Add release-xx label to identify the PR slated for a upcoming release (will be used in release discussion)
  3. Add needs-dev-review label when work is completed
  4. Add needs-qa label when dev review is completed
  5. Add QA Passed label when QA has signed off

Description

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?

Screenshots/Recordings

If applicable, add screenshots and/or recordings to visualize the before and after of your change

Issue

Progresses #460

Checklist

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

@jpcloureiro jpcloureiro requested a review from a team as a code owner November 9, 2022 14:45
@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2022

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.

NicholasEllul
NicholasEllul reviewed Nov 9, 2022
View reviewed changes
Copy link
Contributor

@NicholasEllul NicholasEllul left a comment

Choose a reason for hiding this comment

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

I love this approach of intercepting the request! Are we able to add any tests to catch regressions in this behaviour?

app/components/Views/BrowserTab/index.js Outdated Show resolved Hide resolved
app/components/Views/BrowserTab/index.js Outdated Show resolved Hide resolved
app/components/Views/BrowserTab/index.js Outdated Show resolved Hide resolved
@andreahaku
Copy link
Contributor

andreahaku commented Nov 9, 2022 •
edited
Loading

@jpcloureiro did you check how this change behaves with the app specific schemas like metamask://, dapp://, wc:// or ethereum://?

@jpcloureiro jpcloureiro added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) q4 labels Nov 10, 2022
@jpcloureiro
Copy link
Contributor Author

@jpcloureiro did you check how this change behaves with the app specific schemas like metamask://, dapp://, wc:// or ethereum://?

@andreahaku opening any of these schemas inside the webview will open the alert dialog. If the user presses allow, it will execute as before.

@jpcloureiro
Copy link
Contributor Author

I've change the functionality so that we can have a set of whitelisted protocols that will be opened by the webview without an alert dialog.

Any other protocol will display an alert dialog with the message `This website has been blocked from automatically opening an external application'

@sethkfman sethkfman added the release-5.12.0 Issue or pull request that will be included in release 5.12.0 label Nov 15, 2022
@sethkfman sethkfman removed the release-5.12.0 Issue or pull request that will be included in release 5.12.0 label Nov 18, 2022
sethkfman
sethkfman approved these changes Dec 14, 2022
View reviewed changes
Copy link
Contributor

@sethkfman sethkfman left a comment

Choose a reason for hiding this comment

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

One minor comment for a method rename but other than that it looks good.

app/util/browser/index.test.ts Outdated Show resolved Hide resolved
app/util/browser/index.ts Outdated Show resolved Hide resolved
app/components/Views/BrowserTab/index.js Outdated Show resolved Hide resolved
sethkfman
sethkfman approved these changes Dec 15, 2022
View reviewed changes
Copy link
Contributor

@sethkfman sethkfman left a comment

Choose a reason for hiding this comment

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

LGTM

@sethkfman sethkfman added needs-qa Any New Features that needs a full manual QA prior to being added to a release. Mobile QA board release-5.13.0 Issue or pull request that will be included in release 5.13.0 and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Dec 15, 2022
NicholasEllul
NicholasEllul reviewed Dec 20, 2022
View reviewed changes
app/util/browser/index.ts Outdated Show resolved Hide resolved
app/util/browser/index.ts Outdated Show resolved Hide resolved
@sethkfman
Copy link
Contributor

@jpcloureiro Can you add the scenarios you tested and update the lint errors?

@sethkfman sethkfman added release-5.14.0 Issue or pull request that will be included in release 5.14.0 and removed release-5.13.0 Issue or pull request that will be included in release 5.13.0 labels Dec 22, 2022
@cortisiko cortisiko removed the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Dec 27, 2022
@jpcloureiro jpcloureiro force-pushed the feature/improve-links-experience branch from 96a35ad to 2e989af Compare January 27, 2023 23:48
@jpcloureiro
Copy link
Contributor Author

@jpcloureiro sending this back your way!

Issue

On the webview error screen, when I tap the "return to home page" button I get:[TypeError: Invalid URL: home.metamask.io]. Furthermore, I am not taken back to the mm home page. see recording

To reproduce:

  • open the browser
  • enter an invalid URL. For example https://asfasfo.aox
  • When the error screen appears
  • tap on "return to home page". Notice the error.

@cortisiko pushed a fix. It should go back to mm home page now.

@sethkfman sethkfman added needs-qa Any New Features that needs a full manual QA prior to being added to a release. release-6.1.0 Issue or pull request that will be included in release 6.1.0 and removed team-mobile-client QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed release-5.14.0 Issue or pull request that will be included in release 5.14.0 labels Jan 30, 2023
@cortisiko cortisiko added QA Passed A successful QA run through has been done and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Feb 2, 2023
cortisiko
cortisiko approved these changes Feb 2, 2023
View reviewed changes
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.

Fix looks good @jpcloureiro, This is QA passed! 🌮 🌮

@cortisiko cortisiko merged commit 96466ad into main Feb 2, 2023
@cortisiko cortisiko deleted the feature/improve-links-experience branch February 2, 2023 23:46
@github-actions github-actions bot locked and limited conversation to collaborators Feb 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@cortisiko cortisiko cortisiko approved these changes

@sethkfman sethkfman sethkfman approved these changes

@NicholasEllul NicholasEllul Awaiting requested review from NicholasEllul

Assignees
No one assigned
Labels
QA Passed A successful QA run through has been done release-6.1.0 Issue or pull request that will be included in release 6.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jpcloureiro @andreahaku @sethkfman @cortisiko @NicholasEllul @joaoloureirop