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.

Sign up for GitHub

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

Jump to bottom

Complete redesigns for approval flow #1558

Merged
merged 49 commits into from
Jun 24, 2020
Merged

Complete redesigns for approval flow #1558

merged 49 commits into from
Jun 24, 2020

Conversation

rickycodes
Copy link
Contributor

@rickycodes rickycodes commented May 11, 2020 •
edited
Loading

Description

re: #1503

opening this as a draft PR for now, going to clean things up after I get back from my afternoon walk :)

Okay, I think this is mostly good for initial review, going to leave this as a draft since it relies on #1512

Checklist

  • Merge in @EtDu's work
  • Get components updated based on designs
  • Any added code is fully documented
  • Cleanup

Issue

Resolves #1503

@rickycodes rickycodes added the DO-NOT-MERGE Pull requests that should not be merged label May 12, 2020
@rickycodes rickycodes changed the base branch from develop to connect-design May 13, 2020 15:51
@rickycodes rickycodes changed the base branch from connect-design to transaction-components May 13, 2020 15:53
@EtDu EtDu force-pushed the transaction-components branch 2 times, most recently from 90a451b to d48dc94 Compare May 18, 2020 09:53
@rickycodes rickycodes force-pushed the issue-1503 branch 2 times, most recently from ef21f2a to 9367577 Compare May 20, 2020 14:43
@rickycodes rickycodes changed the base branch from transaction-components to develop May 20, 2020 14:45
@rickycodes rickycodes changed the base branch from develop to transaction-components May 20, 2020 14:45
@rickycodes rickycodes force-pushed the issue-1503 branch 2 times, most recently from ef21f2a to d5e4269 Compare May 20, 2020 23:08
@rickycodes rickycodes requested a review from danjm May 22, 2020 16:46
danjm
danjm reviewed May 26, 2020
View reviewed changes
app/util/transactions.js Outdated Show resolved Hide resolved
danjm
danjm requested changes May 26, 2020
View reviewed changes
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.

Left two comments. The code looks fine overall. All good for me if QA passes.

@rickycodes rickycodes removed the DO-NOT-MERGE Pull requests that should not be merged label May 26, 2020
@rickycodes rickycodes changed the base branch from transaction-components to develop May 27, 2020 14:21
@rickycodes rickycodes changed the base branch from develop to transaction-components May 27, 2020 14:21
@rickycodes rickycodes force-pushed the issue-1503 branch 2 times, most recently from 32a8170 to a902237 Compare June 2, 2020 19:43
@rickycodes
Copy link
Contributor Author

rickycodes commented Jun 3, 2020 •
edited
Loading

Leaving some screenshots here for design QA:

Initial Approve Token Screen:

image

Edit Permissions:

image

Transaction Details:

image

Transaction Details (View Data Expanded):

image

A couple things:

Transaction details is currently missing a row for name as the contract for token approval doesn't have one. name isn't always available. After discussion with @estebanmino he suggested we leave it out for now. The paste icon is still what we were using previously vs the new clipboard treatment that's in the new screens, I wasn't sure if we wanted to change that everywhere? Again, that's something we can look at adding in a separate PR.

@cjeria
Copy link

cjeria commented Jun 3, 2020

Transaction details is currently missing a row for name as the contract for token approval doesn't have one. name isn't always available. After discussion with @estebanmino he suggested we leave it out for now.

Sounds good.

The paste icon is still what we were using previously vs the new clipboard treatment that's in the new screens, I wasn't sure if we wanted to change that everywhere? Again, that's something we can look at adding in a separate PR.

Let's keep the icon used here and elsewhere. I had pulled that clipboard icon from fontawesome but I think the current icon used here works well.

Looks like there added padding/margins to the edit permission subview I noticed. Let's make this consistent with all other padding/margins.
image

@ibrahimtaveras00 ibrahimtaveras00 mentioned this pull request Jun 23, 2020
Merged
4 tasks
@omnat omnat added needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed labels Jun 23, 2020
@ibrahimtaveras00
Copy link
Contributor

All fixes look good 👍

The only issue/suggestion I have is for Network fee, when in fiat mode do we want to stay consistent with other designs and only show up to 2 decimal points?

image

@rickycodes
Copy link
Contributor Author

The only issue/suggestion I have is for Network fee, when in fiat mode do we want to stay consistent with other designs and only show up to 2 decimal points?

This should be good now as well :)

estebanmino
estebanmino approved these changes Jun 24, 2020
View reviewed changes
Copy link
Contributor

@estebanmino estebanmino left a comment

Choose a reason for hiding this comment

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

LGTM!

ibrahimtaveras00
ibrahimtaveras00 approved these changes Jun 24, 2020
View reviewed changes
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.

All fixes look good on both OS's, QA Passed 👍

@ibrahimtaveras00 ibrahimtaveras00 added QA Passed A successful QA run through has been done and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Jun 24, 2020
@rickycodes rickycodes merged commit af69702 into develop Jun 24, 2020
@rickycodes rickycodes deleted the issue-1503 branch June 24, 2020 17:46
@rickycodes rickycodes mentioned this pull request Jun 30, 2020
Closed
rickycodes added a commit that referenced this pull request Jan 31, 2022
@rickycodes @EtDu
Complete redesigns for approval flow (#1558)
b340254 
* Confirm: Use TransactionReviewFeeCard, add primaryCurrency prop, add ETH transaction fee, move renderIfGasEstimationReady to TransactionReviewFeeCard, remove adjust transaction fee button, pass props to TransactionReviewFeeCard

* CustomGas: Implement redesign. Still need to modify styling of individual gas selectors

* snapshot update

* Approve: Use Modal in place of Action Modal, use updated CustomGas

* Confirm: fix, change props.transactionState to state.transaction (merge conflict recovery)

* snapshot update

* Approve: add in ActionModal styles, use ActionModal

* Use ActionModal

* Remove transaction direction

* Add onRequestClose

* Add host and network under website icon

* Use TransactionHeader component

* Use AccountInfoCard

* Add black to text in AccountInfoCard

* Remove up/down arrow icon

* Update: styles

* Update networkFee

* Update: translation text

* Update: sync up with PR and update translations

* Add TransactionReivewDetailsCard

* Include getMethodData

* Rename onViewDetails -> toggleViewDetails

* Adjust styles

* Remove commented code

* Update: tests

* Update: TransactionReviewDetailsCard styles

* Update: Move customGas and editPermission out from Modals

* Update: remove now redundant infos

* Remove comments

* Remove padding

* Fix eslint issues

* Adjust location of KeyboardAwareScrollView for renderEditPermission

* Fix transition when opening network fee

* Update: Create translation for "Total"

* Update: secureIcon

* Update: remove padding

* Update: Add ConnectHeader component

* Update: Make ConnectHeader props isRequired

* Update: move view into RenderCustomGas method

* Add activeTabUrl so we get https and secure icon

* Update: display network fee in fiat when selected

* Update: Add save button to network fee

* Update: Add Set button for editPermission

* Update: tests

* Fix hitTest issue with network fee

* Update: Display currencySymbol

* Update: round fiat price to two decimals

* Update: fallback to  just to be safe

* Update: Add getActiveTabUrl method

Co-authored-by: Etienne Dusseault <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ibrahimtaveras00 ibrahimtaveras00 ibrahimtaveras00 approved these changes

@estebanmino estebanmino estebanmino approved these changes

@danjm danjm danjm approved these changes

Assignees
No one assigned
Labels
QA Passed A successful QA run through has been done
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Complete redesigns for approval flow
7 participants
@rickycodes @cjeria @omnat @ibrahimtaveras00 @danjm @estebanmino @EtDu