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

Confirmations: Empty SiteURL and Null TagURL in modal #6274

Merged
merged 6 commits into from
May 16, 2023
Merged

Confirmations: Empty SiteURL and Null TagURL in modal #6274

merged 6 commits into from
May 16, 2023

Conversation

blackdevelopa
Copy link
Contributor

@blackdevelopa blackdevelopa commented Apr 26, 2023

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
There are two issues that show up on the approve confirmation modal when you deeplink to the app. The first is a null tagURL which is well documented here and the second is an undefined Site URL value well documented here. Following the conversations on the issues and here, here, this PR includes a fix that hides both the tagurl and siteurl when not available.

Screenshots/Recordings

RPReplay_Final1682506644.MP4

Issue

Progresses #5570 and https://github.com/MetaMask/mobile-planning/issues/883

Checklist

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

@blackdevelopa blackdevelopa requested a review from a team as a code owner April 26, 2023 11:48
@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.

@blackdevelopa blackdevelopa self-assigned this Apr 26, 2023
@blackdevelopa blackdevelopa added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) team-confirmations-secure-ux-PR PR from the confirmations team labels Apr 26, 2023
Copy link
Contributor

@jpuri jpuri left a comment

Choose a reason for hiding this comment

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

Hey @blackdevelopa , changes look good. Will be nice to have some test coverage also.

segun
segun previously approved these changes May 3, 2023
@blackdevelopa blackdevelopa added needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels May 3, 2023
@seaona
Copy link
Contributor

seaona commented May 15, 2023

Looks good @blackdevelopa I can see how the URL is not displayed when not available.

image

image

@seaona seaona 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 May 15, 2023
@seaona seaona requested review from segun and jpuri May 15, 2023 14:37
@seaona seaona added the release-6.7.0 6.7.0 release tickets label May 16, 2023
@seaona seaona merged commit 9016b4f into main May 16, 2023
14 checks passed
@seaona seaona deleted the bg/5570 branch May 16, 2023 15:18
@github-actions github-actions bot locked and limited conversation to collaborators May 16, 2023
@cortisiko cortisiko added release-7.1.0 and removed release-6.7.0 6.7.0 release tickets labels May 31, 2023
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.1.0 team-confirmations-secure-ux-PR PR from the confirmations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants