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

feat: add portfolio button to asset action buttons #5785

Merged
merged 22 commits into from
Mar 18, 2023

Conversation

meppsilon
Copy link
Contributor

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

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
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.

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.

Left some comments

app/components/UI/AccountOverview/index.js Outdated Show resolved Hide resolved
app/components/UI/AccountOverview/index.js Outdated Show resolved Hide resolved
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.

translations LGTM

@Cal-L Cal-L added Mobile QA board 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 and removed team-wallet-api-platform labels Feb 27, 2023
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.

@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
Contributor Author

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
Member

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 PR for release 6.3.0 and removed release-6.2.0 labels Mar 13, 2023
@cortisiko
Copy link
Member

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

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.

this PR looks good @meppsilon 🌮 🌮 🌮

@cortisiko cortisiko added the QA Passed A successful QA run through has been done 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 DEPRECATED: please use "team-wallet-ux" label instead label 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-6.3.0 PR for release 6.3.0 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

5 participants