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

Improvement: send flow #1327

Merged
merged 39 commits into from
Feb 27, 2020
Merged

Improvement: send flow #1327

merged 39 commits into from
Feb 27, 2020

Conversation

estebanmino
Copy link
Contributor

Description

Improvements based on @cjeria doc

Checklist

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

Issue

Resolves #???

@estebanmino estebanmino changed the title [WIP] Improvement: send flow Improvement: send flow Feb 7, 2020
@estebanmino estebanmino added the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Feb 7, 2020
@danjm danjm self-assigned this Feb 10, 2020
@omnat omnat added this to the Sprint Feb10 milestone Feb 10, 2020
@omnat omnat mentioned this pull request Feb 10, 2020
2 tasks
@@ -131,6 +133,11 @@ class ContactForm extends PureComponent {
componentDidMount = () => {
const { mode } = this.state;
const { navigation } = this.props;
// Workaround https://github.com/facebook/react-native/issues/9958
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting workaround. I guess we should check back on that react-native issue and remove the workaround once the underlying problem is resolved

Copy link
Contributor

@danjm danjm left a comment

Choose a reason for hiding this comment

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

All the code changes look good to me

@ibrahimtaveras00
Copy link
Contributor

All fixes look good 👍

Last issue I found is more of a question; do we want to have an ellipsis here @estebanmino @cjeria?

Screen Shot 2020-02-25 at 9 24 32 PM

Same as on the send view as well

Screen Shot 2020-02-25 at 9 27 31 PM

@cjeria
Copy link

cjeria commented Feb 26, 2020

Last issue I found is more of a question; do we want to have an ellipsis here @estebanmino @cjeria?

Yes to ellipsis. Let's try to make it as wide as possible.

@omnat
Copy link
Contributor

omnat commented Feb 26, 2020

QA: Recent addresses only the ones from address book, not the ones not saved. This logic needs changing. @danjm

@ibrahimtaveras00
Copy link
Contributor

Ellipsis fix looks good 👍

Screen Shot 2020-02-26 at 3 00 36 PM

Copy link
Contributor

@ibrahimtaveras00 ibrahimtaveras00 left a comment

Choose a reason for hiding this comment

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

Changes look good on both OS's QA Passed 👍

@cjeria
Copy link

cjeria commented Feb 26, 2020

Looking good! One additional change I'd like to suggest is to remove the text "No contacts in your address book" or at least use a grey color since it looks like a tappable text link, although I'm more in favor of just removing it altogether.

image

@ibrahimtaveras00 ibrahimtaveras00 merged commit 9bea1b5 into develop Feb 27, 2020
@ibrahimtaveras00 ibrahimtaveras00 deleted the improvement/send-flow branch February 27, 2020 15:57
rickycodes pushed a commit that referenced this pull request Jan 31, 2022
* warning message

* center

* check iszero

* change alias on save name

* fix margins

* add check

* add memo

* snaps

* missing locales

* update decodeTransferData

* Revert "update decodeTransferData"

This reverts commit dcdfbd1.

* get tx to from data when parsing recent addresses

* add editable state

* new edit

* locale

* delete contact

* edit delerte working

* snaps

* update contacts settings

* test

* rm onblur

* issue 1.2

* issue 3

* issue 8

* fix 8.1

* fix check icon position

* fix android

* snaps

* check for data

* fix parsing

* device isandroid

* fixes

* fix uppercase and long names

* snaps

Co-authored-by: Ibrahim Taveras <ibrahim.taveras@consensys.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-qa Any New Features that needs a full manual QA prior to being added to a release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants