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

Dapp confirmation designs transitions #1620

Merged
merged 38 commits into from
Jul 7, 2020

Conversation

EtDu
Copy link
Contributor

@EtDu EtDu commented Jun 5, 2020

Description

Implement screen transitions for dapp-confirmation-designs

Checklist

  • Implement transitions

@EtDu EtDu requested a review from a team as a code owner June 5, 2020 08:23
@EtDu EtDu changed the base branch from develop to dapp-confirmation-designs June 5, 2020 08:23
@EtDu EtDu marked this pull request as draft June 5, 2020 08:24
@EtDu EtDu force-pushed the dapp-confirmation-designs branch 2 times, most recently from f6d7eb1 to 5bd460d Compare June 11, 2020 13:23
@EtDu EtDu force-pushed the dapp-confirmation-designs-transitions branch from 9710dd6 to ea812f7 Compare June 11, 2020 13:23
@EtDu EtDu force-pushed the dapp-confirmation-designs branch from 4a35777 to 127230c Compare June 18, 2020 07:22
@EtDu EtDu force-pushed the dapp-confirmation-designs-transitions branch from ea812f7 to 3fb0105 Compare June 22, 2020 14:04
@EtDu EtDu force-pushed the dapp-confirmation-designs branch from 127230c to 8c6a73a Compare June 23, 2020 04:17
@EtDu EtDu force-pushed the dapp-confirmation-designs-transitions branch from 3fb0105 to 5658bb2 Compare June 23, 2020 04:19
@EtDu EtDu marked this pull request as ready for review June 23, 2020 04:19
/**
* Width of the device
*/
width: PropTypes.number,
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason for getting this through props instead of calling Dimensions.get('window')?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thought since I computed the width once I could just just pass it through. Is it better to call Dimensions.get('window') or Device.getDeviceWidth()?

Copy link
Contributor

Choose a reason for hiding this comment

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

for these two... I'm thinking if we use it somewhere else in the future is gonna be coupled to whichever parent it uses. I'd say Device.getDeviceWidth() since we have a util for it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

/**
* Height of custom gas and data modal
*/
customGasHeight: PropTypes.number,
Copy link
Contributor

Choose a reason for hiding this comment

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

same question for this, seems like a style passed through props?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This value is created via the onLayout function on the root CustomGas view. It is needed to compute the proper transforms for the animations.

It's needed inside of TransactionEditor as well which is why I didn't just compute it and leave it in CustomGas

!this.state.headerHeight && this.setState({ headerHeight: event.nativeEvent.layout.height });

saveGasInputHeight = event => {
!this.state.gasInputHeight && this.setState({ gasInputHeight: event.nativeEvent.layout.height });
Copy link
Contributor

Choose a reason for hiding this comment

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

hm this could be the answer to my questions before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, saveCustomGasHeight called by onLayout from the root CustomGas view

let xTranslationValue;
if (xTranslationName === 'reviewToEdit') xTranslationValue = reviewToEditValue;
else if (xTranslationName === 'editToAdvanced') xTranslationValue = editToAdvancedValue;
else if (xTranslationName === 'reviewToData') xTranslationValue = reviewToDataValue;
Copy link
Contributor

Choose a reason for hiding this comment

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

I see many constants here, maybe could be a good idea to put these on the top of the file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made an object that maps the strings to the state vars and put it at the top of component

xTranslationName === 'reviewToData' &&
xTranslationEndValue === 0 &&
this.setState({
hideData: true
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like https://github.com/MetaMask/metamask-mobile/pull/1620/files#diff-fc12bc5d7b49a26e971be23e01a8c79cR626 is the same code but the boolean than here ... can we put that in a method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

style={[
styles.root,
this.generateTransform('modal', [
Device.isIphoneX() ? rootHeight - customGasHeight : rootHeight - customGasHeight + 24,
Copy link
Contributor

Choose a reason for hiding this comment

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

I beleieve I saw this 3 times on this file, can we use a method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@ibrahimtaveras00 ibrahimtaveras00 added the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Jun 25, 2020
@EtDu EtDu force-pushed the dapp-confirmation-designs-transitions branch from c4ec334 to 47e924d Compare June 25, 2020 16:13
@ibrahimtaveras00
Copy link
Contributor

Issue 1 from #1559:

I now see that android is having issues with the keyboard blocking the text input field; seen here = http://recordit.co/FNMCosZIMK

Also in the above screencast notice how the padding is off on both OS's

Screen Shot 2020-07-01 at 3 57 28 PM

Screen Shot 2020-07-01 at 5 30 55 PM

I'm not sure if this is related to this fix/change, but on normal send flow, when I tap the Edit button to the right of Network fee nothing happens

Screen Shot 2020-07-01 at 5 46 56 PM

Issue 2 from #1559 seems resolved here 👍

Issue 4 from #1559 seems resolved here 👍

Other Issues on this PR

  • I'm noticing the padding is off when I tap on View Data button

Screen Shot 2020-07-01 at 3 54 10 PM

I also tried to check if approve shows the hex data but I see the old approve still (assuming develop needs to be merged into this PR and I see a conflict above)

Screen Shot 2020-07-01 at 5 44 15 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.

few issues reported

@ibrahimtaveras00 ibrahimtaveras00 added QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Jul 1, 2020
@EtDu EtDu force-pushed the dapp-confirmation-designs branch from 7831366 to 87105b5 Compare July 2, 2020 03:52
@EtDu EtDu force-pushed the dapp-confirmation-designs-transitions branch from 273b0af to eccdabc Compare July 4, 2020 07:10
@EtDu EtDu force-pushed the dapp-confirmation-designs branch 2 times, most recently from 4f6d250 to e1c0f24 Compare July 5, 2020 04:56
@EtDu EtDu force-pushed the dapp-confirmation-designs-transitions branch from 97c95f9 to 03ac0c3 Compare July 5, 2020 05:25
EtDu added 25 commits July 7, 2020 12:01
@EtDu EtDu force-pushed the dapp-confirmation-designs-transitions branch from d0d40d8 to 008cbc1 Compare July 7, 2020 04:02
@EtDu EtDu merged commit 0dae07f into develop Jul 7, 2020
@EtDu EtDu deleted the dapp-confirmation-designs-transitions branch July 7, 2020 04:11
rickycodes pushed a commit that referenced this pull request Jan 31, 2022
* Move handleFetchBasicEstimates from CustomGas to TransactionEditor, pass down basicGasEstimates, can only edit gas from review screen if basic estimates done, instead of calculating after pressing edit & showing a loader

* CustomGas: compute height of root view and sent to TransactionEditor via saveCustomGasHeight()

* TransactionEdit: pass through saveCustomGasHeight

* TransactionEditor: Implement animations for review --> edit mode, remove redundant functions goToEdit & handleCustomBackPress

* TransactionEditor & CustomGas: move advancedCustomGas state variable to TransactionEditor

* TransactionEditor: invert start & end animation values, 0 --> 1 to 1 --> 0

* TransactionEditor: use a unique animation value for root modal: y axis, custom gas: x axis, advanced custom gas: x axis, animation will always be root modal + (custom gas OR advanced custom gas)

* TransactionEditor/CustomGas: enable vertical modal animation for edit --> advanced

* TransactionEditor: Modal moves to correct height from review --> edit if advanced is selected

* TransactionEditor/CustomGas: create proper animation for edit --> advanced custom gas, give selectors absolute positioning, hide them when they are off the screen

* TransactionEditor/CustomGas: All animations work together

* TransactionEditor/CustomGas: animation works identically on non iphoneX ++ devices

* TransactionEditor: change animation time to 250ms, apply easing

* TransactionReviewData: ensure data box is correct height

* CustomGas & TransactionEdit: pass through customGasHeight & rootHeight, clean up in CustomGas

* TransactionReview: add in animated views

* TransactionEditor: implement animation for transaction data, refactor some animation logic

* TransactionReview: fix invalid proptype

* Test fixes

* TransactionEditor: set default gas fee to average on mount

* TransactionReviewInformation: fix gas not updating after saving selection from selectors

* CustomGas: fix advanced CG total not reflecting the one selected via the selectors

* TransactionEditor: map xtranslation strings with state vars

* TransactionEditor: use hideComponents method

* TxEditor & CustomGas: use getTransformValue to avoid repetition

* TransactionEditor: fix animation glitch with tx data

* CustomGas: add gas price extremely low error, but don't disable the button because of it

* Use Device.getDeviceWidth instead of passing width as props

* TransactionEditor: use keyboard aware scrollview

* snapshop update

* yet another snapshot update yo

* allow explicitly hiding or showing dappTransactionModal, hide on Nav/Main mount explicitly

* CustomGas: modify styles to fix padding

* TransactionReviewFeeCard: fix disabled edit button, fix paddings for custom gas

* TransactionReviewData: fix padding

* snapshot updatessss

* snapshot to the moon

* CustomGas: display basic gas estimate avergae gas price by default on advanced custom gas editor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next release QA Passed A successful QA run through has been done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants