Skip to content

feat: add portfolio button to asset action buttons#5785

Merged
cortisiko merged 22 commits intoMetaMask:mainfrom
meppsilon:me/entry-point
Mar 18, 2023
Merged

feat: add portfolio button to asset action buttons#5785
cortisiko merged 22 commits intoMetaMask:mainfrom
meppsilon:me/entry-point

Conversation

@meppsilon
Copy link
Copy Markdown
Contributor

@meppsilon meppsilon commented Feb 17, 2023 •
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

This PR adds a button to the wallet view that, when clicked, opens the browser to the portfolio dapp. We hope that this feature improves mobile users experience by providing them with quick access to additional portfolio-specific views.

Screenshots/Recordings

Screenshot 2023-03-09 at 4 36 23 PM

Issue

Progresses #???

Checklist

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

@meppsilon meppsilon requested a review from a team as a code owner February 17, 2023 22:17
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Feb 17, 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.

Cal-L
Cal-L reviewed Feb 21, 2023
View reviewed changes
Copy link
Copy Markdown
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.

Left some comments

Cal-L
Cal-L approved these changes Feb 27, 2023
View reviewed changes
Copy link
Copy Markdown
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.

translations LGTM

@Cal-L Cal-L added Mobile QA board release-6.2.0 Issue or pull request that will be included in release 6.2.0 needs-qa Any New Features that needs a full manual QA prior to being added to a release. team-wallet-api-platform-deprecated DEPRECATED: please use "team-wallet-integrations" instead and removed team-wallet-api-platform-deprecated DEPRECATED: please use "team-wallet-integrations" instead labels Feb 27, 2023
cortisiko
cortisiko reviewed Mar 2, 2023
View reviewed changes
Copy link
Copy Markdown
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.

@meppsilon questions for you:
1.
Should a new browser tab always open when I tap on the portfolio button? Right now if I were to connect to the pdapp via the portfolio button > return to the wallet > tap on the portfolio button again, a new browser tab opens up. If I repeat this n times then I will have n browser tabs opened for the portfolio dapp.

See recording

2
Maybe this is something the design team can answer:

With languages such as Russian or Filipino, there are ellipsis that prevents the users from seeing the full text on the buttons

See prod behavior vs Your branch

@cortisiko cortisiko added QA'd but questions A QA run through has been done but you need clarification on minor issues you found and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Mar 2, 2023
@meppsilon
Copy link
Copy Markdown
Contributor Author

meppsilon commented Mar 6, 2023

Hey @cortisiko,

  1. I think that's the expected behavior for now. In a future release, we might try to redirect the user to an existing browser tab but for now, this simple behavior is expected.
  2. Yea, that is a better question for design but I'm not sure if there is a way around this unless we wrap the buttons around to another level.

@cortisiko
Copy link
Copy Markdown
Member

cortisiko commented Mar 9, 2023

hey @meppsilon following up on our conversation in the release planning this week, did we come to a conclusion on issue 1?

@cortisiko cortisiko removed the QA'd but questions A QA run through has been done but you need clarification on minor issues you found label Mar 9, 2023
@cortisiko cortisiko added release-6.3.0 Issue or pull request that will be included in release 6.3.0 and removed release-6.2.0 Issue or pull request that will be included in release 6.2.0 labels Mar 13, 2023
@cortisiko
Copy link
Copy Markdown
Member

cortisiko commented Mar 16, 2023

Alrighty @meppsilon I tested this PR again. The good news is, the browser tab does not open multiple tabs when I tap the pdapp button.

The bad news:
If I connect to the pdapp and then open another tab, upon returning to the wallet view and tapping the pdapp icon again, instead of being taken to the pdapp, I am redirected to the current active browser tab: http://recordit.co/KO5GulaBpx

I also observed that if the pdapp is open in a separate browser tab while another dapp or website is currently active, and I close and relaunch the pdapp, upon tapping on the browser button, the pdapp automatically opens up: http://recordit.co/0olVVc2d4f

In prod this is not the case. With your PR, this changes the way the browser works. See behavior in prod: http://recordit.co/cizlY3qsJI

cortisiko
cortisiko approved these changes Mar 18, 2023
View reviewed changes
Copy link
Copy Markdown
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.

this PR looks good @meppsilon 🌮 🌮 🌮

@cortisiko cortisiko added the QA Passed QA testing has been completed and passed label Mar 18, 2023
@cortisiko cortisiko merged commit 4fc9881 into MetaMask:main Mar 18, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Mar 18, 2023
@gauthierpetetin gauthierpetetin added the team-mobile-ux Mobile UX team label Feb 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@Cal-L Cal-L Cal-L approved these changes

@cortisiko cortisiko cortisiko approved these changes

Assignees

No one assigned

Labels

QA Passed QA testing has been completed and passed release-6.3.0 Issue or pull request that will be included in release 6.3.0 team-mobile-ux Mobile UX team

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@meppsilon @cortisiko @Cal-L @gauthierpetetin @sethkfman