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

Address book send #6681

Closed
wants to merge 55 commits into from
Closed

Conversation

chikeichan
Copy link
Contributor

@chikeichan chikeichan commented Jun 4, 2019

  • Implement new send to-autocomplete UI
  • Implement new send flow - address list view
  • Implement new send flow - send content view
  • Fix confirm flow if necessary
  • Implement "Add to Address" book modal
  • Implement Error/Loading state for to-autocomplete
  • Add/Fix unit tests
  • Add/Fix integration tests

@danjm danjm mentioned this pull request Jun 17, 2019
@danjm danjm self-assigned this Jun 17, 2019
@bdresser bdresser added this to the UI Sprint 15 [June 24] milestone Jun 24, 2019
@chikeichan chikeichan marked this pull request as ready for review June 25, 2019 02:35
@bdresser
Copy link
Contributor

@chikeichan this looks awesome!! could you record a short demo of the flow in the extension popup when you get a chance? will help with design QA

@chikeichan
Copy link
Contributor Author

@chikeichan this looks awesome!! could you record a short demo of the flow in the extension popup when you get a chance? will help with design QA

https://www.loom.com/share/cd3cae924c3748f5ba490eda2de3e801

There you go!

For this one, please still pull it down and play with it locally if possible 🙇 . This is a major refactor of the send flow, so we want to be very cautious.

@chikeichan chikeichan force-pushed the address-book-send branch 2 times, most recently from 02be7d1 to 9adc45e Compare June 26, 2019 19:44
@cjeria
Copy link
Contributor

cjeria commented Jun 26, 2019

Nice work @chikeichan! Here's a list of some feedback I caught while testing.

  1. When clicking “send” from the account view, focus cursor in the to field (if possible) for faster recipient inputting.

  2. Make section header say "add recipient” by default. Change to “Send” when ETH address detected.

2.1 Update placeholder text to the suggested text in designs

image

  1. Make recipient name 14 pt font size
    image

  2. Behavior when pasting an ethereum address, don’t make me have to click the list item below the to field. Auto show the send view.
    image

  3. If no other tokens in my wallet, don’t make this field a dropdown
    image

  4. In order to avoid this overlap text, we agreed that it would be fine to ellipsis the middle part of an address like "0x28b…6581"
    image

  5. In full-screen mode, let’s fix the grey bar color so it blends with the rest of the background
    image

  6. Are we able to show the saved contact’s name in the confirm screen (if contact was saved)?
    image

  7. Give each row here a top padding of 16px
    image

  8. some addresses overlap the "X" here, not sure why? I believe Roboto is mono-spaced...
    image

  9. Saving is not working consistently, I was only able to save one contact the first time only.

I realize this might be an early stage version of the feature, so please let me know what's still being worked on. cc @bdresser

@danjm
Copy link
Contributor

danjm commented Jun 27, 2019

@chikeichan I think it would be appropriate to remove the test-integration-flat-chrome send test at this point, as long as create an issue that documents the behaviour covered by those tests that is not covered by e2e tests. We need to replace these tests with additions to the e2e tests eventually, and this is a significant enough change that I think now is the appropriate time.

}
}

export default connect(
Copy link
Contributor

Choose a reason for hiding this comment

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

In most places we follow the pattern of creating a .container.js file for connection to redux. Do you have a preference for doing this within the same file that defines the component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@danjm
Copy link
Contributor

danjm commented Jun 27, 2019

I think the code looks good generally. I need to take one more close look at the ens-input changes.

@danfinlay
Copy link
Contributor

This is awesome! It looks great.

A question:

  • Is there a way to copy the address after the ENS name has resolved?

Some notes for future improvement, I could open as design issues, would that be right, @cjeria?

  • Confirmation screen does not show the account nickname like the send form did.
  • The nickname should appear somewhere in the TX history list, when we have a nickname for who we sent to.

@cjeria
Copy link
Contributor

cjeria commented Jun 27, 2019

@danfinlay

  • Is there a way to copy the address after the ENS name has resolved?

Which screen/step are you wanting to copy the address?

Some notes for future improvement, I could open as design issues, would that be right, @cjeria?

Sure! Let's create a new issue in design for future enhancements to this feature.

@danfinlay
Copy link
Contributor

Opened new issues #105, #106, maybe should've been one issue, feel free to consolidate.

@bdresser
Copy link
Contributor

@chikeichan this looks awesome! super smooth

Biggest thing I'm noticing is that addresses should be network-specific. If I save a contact on Mainnet, it's not necessarily a testnet address, so showing it in the testnet Send flow could cause a lot of user errors. Let me know if you want to chat about this live or if we should pull @jennypollack in as well.

Some smaller notes:

  • I think the "copy" request is in scope for this. On the confirm screen, the user should be able to copy the address their sending to (whether raw address, contact, or ENS name) by hovering over the address field. We can reuse the existing Copy Address component (see below). Many folks are religious about double double checking.

Screen Shot 2019-06-28 at 11 33 18 AM

  • On the "Add Contact" screen (below):

Screen Shot 2019-06-28 at 11 28 16 AM

  • pressing enter on the "add contact" screen should save the contact
  • we should auto-focus cursor inside the text field when this modal opens

Also agree with Christian's notes above. Happy to set something up for next week if you want to go over any of this live!

@chikeichan
Copy link
Contributor Author

chikeichan commented Jul 3, 2019

@cjeria

  1. Fixed
  2. Fixed
  3. Fixed
  4. Fixed
  5. Created issue Asset Dropdown should not be openable if the account does not have any tokens added #6787
  6. Fixed
  7. Fixed
  8. Created issue Confirm screen header should show recipient's alias if available #6788
  9. Fixed
  10. Roboto is not monospaced. Roboto Mono is monospaced.
  11. Tagging @jennypollack here for attention re: saving inconsistency.

@danjm

  • Refactor the naming to follow .container.js/.component.js
  • Would love some help on integration test if possible. Been trying to make it pass in CI and locally...

@bdresser

  • "clicked to save" is already there on confirm screen
  • Created issue Addressbook should be Network-specific #6789 to make address book network-specifc
  • Added autofocus for AddAddressModal
  • Added "Press Enter to Save" for AddAddressModal

@chikeichan chikeichan force-pushed the address-book-send branch from 6b7e1ea to 4565d5e Compare July 3, 2019 01:53
@chikeichan chikeichan requested a review from Gudahtt as a code owner July 5, 2019 12:02
danjm
danjm previously approved these changes Jul 5, 2019
@danjm
Copy link
Contributor

danjm commented Jul 24, 2019

@cjeria Improved error messaging related to ENS:

enserrors

@danjm danjm force-pushed the address-book-send branch from 414ed87 to 4578c87 Compare July 25, 2019 15:05
@whymarrh
Copy link
Contributor

Closed by #6914

@whymarrh whymarrh closed this Jul 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants