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

Show a 'send eth' button on home screen in full screen mode #9845

Merged
merged 1 commit into from
Nov 12, 2020

Conversation

darkwing
Copy link
Contributor

Explanation:

Presently there's no "Send ETH" button because we aren't passing a token address for the first item.

Manual testing steps:

  • Open MM in full screen mode
  • See the new button
  • Click it, ensure sending works

SendEth

@darkwing darkwing requested a review from a team as a code owner November 10, 2020 18:23
@darkwing darkwing requested a review from brad-decker November 10, 2020 18:23
@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.

@brad-decker
Copy link
Contributor

brad-decker commented Nov 10, 2020

This was implemented to match designs, I believe the reasoning being that there is already a 'send' button in the eth asset header. @rachelcope thoughts? https://www.figma.com/file/b8U583mpXW1FsPWfGFNT8C/Wallet-home-(popup-%2B-fullscreen)?node-id=217%3A103

@darkwing
Copy link
Contributor Author

As a user, I feel somewhat trained to look at that right column to send. Without this send button, it feels like something is broken or missing.

@rachelcope
Copy link

As @brad-decker said, I recall we left off the "Send Eth" since there's already the call to action in the header. That said, I'm good with this change. I think the consistency of having those CTAs on the right side of the asset list make sense!

🚢

@rachelcope
Copy link

Actually, after looking back at the designs, I think the intent was to show the "Send ETH", "Send DAI", etc. on the hover state.

Not sure if we want to make that update or what you think of it. But it does keeps the UI clean with fewer CTAs but makes the action still possible from the home screen.

I'm also good with the original change suggested in this PR, to keep it simple for now.

@brad-decker
Copy link
Contributor

Actually, after looking back at the designs, I think the intent was to show the "Send ETH", "Send DAI", etc. on the hover state.

Wow, whoops! I totally didn't catch that. I think that should be handled in a separate PR then?

Copy link
Contributor

@brad-decker brad-decker left a comment

Choose a reason for hiding this comment

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

G2G! Thanks, @darkwing for catching this!

@rachelcope
Copy link

Wow, whoops! I totally didn't catch that. I think that should be handled in a separate PR then?

Yes, that makes sense to me!

@darkwing darkwing merged commit 64657ef into MetaMask:develop Nov 12, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Nov 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants