Sig request design fixed #1493
Merged
Sig request design fixed #1493
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@cjeria What do you think? This is with typed data signing. |
|
@EtDu Does the style of the data change per the data type (string vs type data)? For instance, if the message is a string (like the cryptokitties example below), does the style of the message font change? If so, I'm inclined to suggest styling
Also, note that the arrow symbol ">" (indicating the message can be expanded) is only intended for very long messages that don't fit inside the container. |
|
CryptoKitties example with a regular message |
|
|
danjm
requested changes
Apr 21, 2020
| @@ -161,5 +165,9 @@ ActionView.propTypes = { | |||
| /** | |||
| * Determines if the keyboard should stay visible after a tap | |||
| */ | |||
| keyboardShouldPersistTaps: PropTypes.string | |||
| keyboardShouldPersistTaps: PropTypes.string, | |||
| /** | |||
There was a problem hiding this comment.
See above comment on suggested variable name change. The comment here could then be more generalized as well.
There was a problem hiding this comment.
This one was there already
|
I'll do final review once unit tests are passing and we get design approval. I'll also do a QA pass as Ibrahim is mostly out this week. |
estebanmino
reviewed
Apr 22, 2020
estebanmino
reviewed
Apr 22, 2020
| @@ -43,28 +41,33 @@ export default function ActionView({ | |||
| showConfirmButton, | |||
| confirmed, | |||
| confirmDisabled, | |||
| keyboardShouldPersistTaps = 'never' | |||
| keyboardShouldPersistTaps = 'never', | |||
There was a problem hiding this comment.
what's the purpose of this? I remember being debugging some problems for ActionViews that are using this in order to work
There was a problem hiding this comment.
I don't know why this is showing up in the diff, this wasn't my work so I'm not sure
estebanmino
reviewed
Apr 22, 2020
estebanmino
reviewed
Apr 22, 2020
estebanmino
reviewed
Apr 22, 2020
| const { children, currentPageInformation, shouldRenderArrow, toggleExpandedMessage } = this.props; | ||
| const url = currentPageInformation.url; | ||
| const title = getHost(url); | ||
| const arrowIcon = shouldRenderArrow ? this.renderArrowIcon() : null; |
There was a problem hiding this comment.
am I missing the definition of renderArrowIcon ?
There was a problem hiding this comment.
It is directly below renderActionViewChildren
estebanmino
reviewed
Apr 22, 2020
| @@ -229,20 +224,17 @@ class SignatureRequest extends PureComponent { | |||
| confirmText={strings('signature_request.sign')} | |||
| onCancelPress={this.onCancel} | |||
| onConfirmPress={this.onConfirm} | |||
| confirmButtonMode="sign" | |||
| noScroll | |||
There was a problem hiding this comment.
seems like this is removing the ability to scroll? we have to keep in mind that there are smaller devices that needs this to see the whole view
estebanmino
reviewed
Apr 22, 2020
estebanmino
reviewed
Apr 22, 2020
| @@ -43,28 +41,33 @@ export default function ActionView({ | |||
| showConfirmButton, | |||
| confirmed, | |||
| confirmDisabled, | |||
| keyboardShouldPersistTaps = 'never' | |||
| keyboardShouldPersistTaps = 'never', | |||
| noScroll | |||
There was a problem hiding this comment.
what's the purpose to block the abiolity to scroll? we have to support smaller devices that without this could not be able to display all the information we may need to
estebanmino
suggested changes
Apr 22, 2020
There was a problem hiding this comment.
left some comments. @danjm I'd wait for @ibrahimtaveras00 to do QA he can build the app on both IOS and Android devices
|
I'm also worried about the task |
@estebanmino The reasoning for this is because of the new design. Instead of scrolling on the signature request screen, the user would tap the message to expand it (revealing |
EtDu
force-pushed
the
sig-request-design-fixed
branch
from
9c061ec to
8f5eec4
Compare
danjm
force-pushed
the
transaction-header
branch
from
935177b to
6e8d98a
Compare
danjm
force-pushed
the
sig-request-design-fixed
branch
2 times, most recently
from
8f5eec4 to
b449fb2
Compare
|
|
|
with |
estebanmino
added
next release
needs-qa
|
Issue 1: There are quite a few screen sizes that don't play well with the new design:
Issue 2: on iOS (i have an iPhone 6S+ plugged in my macbook, this is left in the screen cast) I don't see the arrow indicating that I can expand the modal, nor when I tap on it, nothing happens. On android (right on the screen cast) when I scroll all the way down, there is no padding on the bottom which makes .html appear cut off a little seen here = http://recordit.co/lOHTAad4dY
this is on https://danfinlay.github.io/js-eth-personal-sign-examples/ with the "personal_sign" example Also, when I'm on android and I tap the native back button on the device, it completely dismisses the modal instead of taking me back to the initial "Sign this message?" view when I have the message expanded. @cjeria should we enable the device back button to dismiss the expanded message view to take us back one step? In addition, in the mock up I see the "Read more" link but not currently implemented (was this intentional)?
|
ibrahimtaveras00
added
the
QA'd - Issues Found
|
@ibrahimtaveras00 Thank you for the feedback, I'll get it fixed |
|
@ibrahimtaveras00 Fixed all of the above! Also, Android native back button takes you back to "Sign this message" |
…splay the account name and address
…, configure expanded signing modal to go back on android back button press, pass down props to signing components
…oggleExpandedMessage
…fix text clipping mid-line
EtDu
force-pushed
the
sig-request-design-fixed
branch
from
63dac66 to
4deab30
Compare
|
Okay I've rebased and switched the compare branch to |
estebanmino
approved these changes
May 7, 2020
estebanmino
added a commit
that referenced
this pull request
May 15, 2020
* Loosen nvmrc (#1524) Co-authored-by: Esteban Miño <efmino@uc.cl> * bugfix/check for sai method (#1545) * update docs link in readme (#1521) Co-authored-by: Esteban Miño <efmino@uc.cl> * Add settings to nav bar (#1544) * Add settings to nav bar * Remove settings Icon * Add icons for all drawer links * Improvement/tx status notification (#1475) * simple not update * text update * wip * delete old details * delete old confirm * almost done withtx details * modal working * modal title * rm transfer element * clean * fix transfer * transfer and payment channel * decodeTransferFromTx * decodeDeploymentTx * decodeConfirmTx * onpress * status * close on view web * more cleanup * payment * showing not * closer? * comment * tx details and not * animated * tx not * enable access view on not * animated * rename * using txnnot manager * working * receive * rm unused * rm logs * handle browser not * parse date * handle asset details * tx summary rename props * Refactor names in details * handle primary currency * missing props * almost there * working but browser * finally * one more thing * done * snaps * missing locales * update ethereum address * snaps * handle instapay txs * snaps * feeless tx * data check * No fee * instance._hideTransactionNotification * fix instapay notifications * elevation * fix remaining issues * apeed up cancel * transaction modal * speed cancel * speedup cancel ui * working * added engine methods * done * snaps * fix qaing * fix ios build * one snap * remove test * status text fix * cancelled * margin * snaps * fix insufficient funds * doc * Transaction Header Component (#1487) * Remove redundant imports, remove redundant styles, comment typo correction, remove renderPageInformation(), split props line by line in render(), swap rendering renderPageInformation() with TransactionHeader component, pass props * added lock and warning icons * removed domain prop * new TransactionHeader component, imports, styling, prop types, lock/warning icon change based on URL protocol, network status indicator (color) changes if network is online/not online * re-generated snapshot for SignatureRequest, created new test for TransactionHeader * network changes based on selected network * update snapshot * update snapshot * remove function, use css for network capitalization * move network status logic to renderNetworkStatusIndicator() * render icon logic moved to renderSecureIcon() * add comments * update snapshot * remove redundant getTrackingParams, use props directly * remove png icons from image dir, use react native svg icons (FontAwesome), update snapshot * TransactionHeader: use 'Ethereum' instead of 'Mainnet' * Add shortnames to networks util, TransactionHeader: use networks util to display network name, update snapshot * Remove redundant imports, remove redundant styles, comment typo correction, remove renderPageInformation(), split props line by line in render(), swap rendering renderPageInformation() with TransactionHeader component, pass props * added lock and warning icons * removed domain prop * new TransactionHeader component, imports, styling, prop types, lock/warning icon change based on URL protocol, network status indicator (color) changes if network is online/not online * re-generated snapshot for SignatureRequest, created new test for TransactionHeader * network changes based on selected network * update snapshot * update snapshot * remove function, use css for network capitalization * move network status logic to renderNetworkStatusIndicator() * render icon logic moved to renderSecureIcon() * add comments * update snapshot * remove redundant getTrackingParams, use props directly * remove png icons from image dir, use react native svg icons (FontAwesome), update snapshot * TransactionHeader: use 'Ethereum' instead of 'Mainnet' * Add shortnames to networks util, TransactionHeader: use networks util to display network name, update snapshot * fixed import error Co-authored-by: Esteban Miño <efmino@uc.cl> * Feature/block screenshots (#1495) * native * test on wallet * block in some screens * ios check * rm asyncs * helper * missing places * Detect if site has been added to Favorites (#1538) * Detect if site has been added to Favorites Previously we were `Alert`ing after attempting to `addBookmark`. Instead, we remove the option from the menu entirely. closes: #1511 * Rename isFavorite -> isBookmark * Use "web-search" keyboardType on iOS (#1539) * Use web-search keyboard Use web-search for the omnibar keyboard * Add new mobile provider (#1517) Co-authored-by: Esteban Miño <efmino@uc.cl> * Sig request design fixed (#1493) * new folder for AccountInfoCard component, remove signature_request.title from message, personal & typed sign components, remove redundant style * SignatureRequest: remove account information from top, proptypes, props, styles, imports * SignatureRequest: change signing message to 'Sign this message?', make bold and larger * remove keyboard aware scroll view * Add AccountInfoCard component to SignatureRequest * AccountInfoCard: implement proper styling * AccountInfoCard: use renderShortAddress, fix styles, use conversionRate to display $ amount * ActionView: add isSigning prop, disables scroll when true, SignatureRequest: pass isSigning=true to ActionView * AccountInfoCard: remove top level view * SignatureRequest: apply styles & layout, add website + arrow icons * Signing components: update styling, rename informationRow --> informationCol * remove message style * TypedSign: put back message style, add messageWrapper style, ensure data fits in box and hides overflow * AccountInfoCard: add snapshot test * update snapshots * styling of 'sign this message' * update snapshot * update snapshot * SignatureRequest: Always render arrow if children coming from TypedSign * SignatureRequest: change to regular component with state to show expanded message content, wrap touchableWithoutFeedback around the message children and move rendering to renderActionViewChildren, tapping the message currently does nothing * fix dapp icon style, fix render inf loop * remove textwrap for typed sign. Now renders properly for V1, V3 & V4 * AccountInfoCard: fix paddings, identicon style, use widths instead of flex, float address to left, fix font weights * TypedSign: use width instead of flex * SignatureRequest: remove website icon wrapper, fix arrow positioning, remove assetLogo style * temp disable warning to match style, ensure message fits within box * PersonalSign: create message wrapper, ensure message fits within box * Message & Personal Sign: use ellipses mode for text wrapper, drop messageWrapper styles * SignatureRequest: remove shouldRenderArrow, add shouldRenderArrow prop * MessageSign: change to stateful component, add renderArrow state, decides if should render arrow upon text component layout, then adjusts the text accordingly * PersonalSign: change to stateful component, add renderArrow state, decides if should render arrow upon text component layout, then adjusts the text accordingly * TypedSign: shouldRenderArrow always passes down as true. Will never be a situation where an entire typed message fits in the box * SignatureRequest: change back to pure component, change handleMessageTap into prop * change handleMessageTap to toggleExpandedMessage * TypedSign: implement message expansion and retraction * modify message, add message_from * new ExpandedMessage component, rendered by typed, personal & regular message components * TypedSign: use ExpandedMessage component * ExpandedMessage: test * SignatureRequest: add renderArrow prop, make box not expandable if false * MessageSign: implement message expanding and retracting * PersonalSign: implement message expand & retract * ExpandedMessage: add mock fns, update all snapshots * ActionView: get rid of top border * new button styles * signing components: ensure a top left and right rounded border * change Cancel & Sign to lowercase * snapshot update * adjust style for android * use unique button style for signing components, fix percentage in stylesheet * change isSigning prop to noScroll * snapshot update * update more snapshots * Signing components: revert to pure component * AccountInfoCard: use weiToFiat & hexToBn helpers to display fiat value, add currentCurrency prop * Signing components: shift renderRootView() contents into render() * update snapshot * AccountInfoCard, SignatureRequest: fix paddings per design * AccountInfoCard: remove bottom margin * TransactionHeader: remove margins, use padding * MessageSign: larger min height, showWarning prop * WarningMessage: use flexstart alignment for bell icon * locales: change eth_sign_warning * WarningMessage: add object as secondary prop type for warning message * SignatureRequest: use WarningMessage component, fix paddings, use renderWarning as prop for WarningMessage * snapshot update yo * SignatureRequest: move AccountInfoCard ontop of message children * snapshot update * AccountInfoCard: remove width * ActionView: remove no scroll - small devices * Signing Components: remove root style, move to SignatureRequest * SignatureRequest: remove style redundancies, add in root style * SignatureRequest: fix height of modal based on screen height * ExpandedMessage: fix styling, move scrollview to signing components renderMessage * ExpandedMessage: Put the scroll back * PersonalSign: remove expandedMessage text wrapper * SignatureRequest: fix up styling, add more overhead (reduced from signing components) * Signing components: reduce view hierarchy, move to SignatureRequest * Locales: add Read more to signature_request * AccountInfoCard: add operation prop, if operation is signing, only display the account name and address * TypedSign: add shouldTruncateMessage & truncateMessage in state * PersonalSign: remove console log * Signing components: change renderArrow to truncateMessage * Nav/Main: add showExpandedMessage to state, add toggleExpandedMessage, configure expanded signing modal to go back on android back button press, pass down props to signing components * Signing Components: move showExpandedMessage out of state, move out toggleExpandedMessage * TypedSign: create different messageWrapper height for iOS & Android, fix text clipping mid-line * ExpandedMessage: fix scrollview * snapshot update * AccountInfoCard: use getTicker * Signing components: change margin bottom from 5 to 4 * Device: new getMediumDevice, SignatureRequest: use getMediumDevice * ActionView, styledButtonStyles: add cancel button style for signing components * snapshot update * SignatureRequest: fix the domain logo not being a circle * update snapshot * Use gaba@1.11.0 (#1552) * Fix settings everywhere (#1556) * Fix day and month numbers in toDateFormat (#1557) * Make send flows consistent (#1465) * Move components and styles from Confirm into TransactionReview * Add ActionView back in * Add missing styles * Revert TransactionReview changes * Update send screen: from accounts editable and redesign gas edit link * Use sendflow confirm for payment requests and when editing * Update sendflow/confirm tests * Use new send flow designs for instapayment / payment channel transactions * Use existing send flow screens for deep link transactions * Fix editing of payment request transactions * Fix unit tests on consistent-send-flow branch * Fix navigation for deep link tx edits on the amount screen. * Refactor: combine transaction and newTransaction reducers * Fix bugs on consistent-send-flow * Fix confirm and edit of transactions created from dapps * Update transaction edit text color * Only allow changing from field on confirm screen of payment requests * Fix amount validation for payment channel transactions * Fix qr payment requests, payment channel payment requests, and token payment requests; plus other small fixes * Fix token approvals * Fix sending of decimals on payment channles * Show correct error messages when accounts are changed and/or token balances are insufficient * Update navbar options in edit mode * Ensure tokens cannot be sent in cases where user has not added the token * Correctly validate payment channel transaction on mount/update * Use sai.svg instead of dai.svg Co-authored-by: Dan Miller <danjm.com@gmail.com> Co-authored-by: Esteban Miño <efmino@uc.cl> * Use setTimeout hack (again) to get paste context in token search (#1548) * Use setTimeout hack (again) to get paste context in token search * Update test Co-authored-by: Esteban Miño <efmino@uc.cl> * V0.2.16 (#1561) * bump * changelog * rm entry * Fix amount validation for large token payment requests (#1572) * Fix validating of amount when sending a collectible (#1565) * Fix validating of amount when sending a collectible * Validate collectible ownership on amount screen. * Ensure correct updating of collectible transaction after edit on the amount screen * Ensure collectibles that use 'transfer' method show a fee in tx history list (#1574) Co-authored-by: Esteban Miño <efmino@uc.cl> * Disable confirm screen edit button when no tokens of a payment request (#1570) * Disable confirm screen edit button when account has no tokens of a payment request * Ensure account switching from undefined token balance accounts enables edit on pay reqs * Improve logic in componentDidUpdate of send/index.js * v0.2.16 changelog (#1575) * Instapay deposit navbar cancel (#1582) * fix * works * rm log * add this to changelog and update date * amount title Co-authored-by: Erik Marks <25517051+rekmarks@users.noreply.github.com> Co-authored-by: Jenny Pollack <jennypollack3@gmail.com> Co-authored-by: ricky <ricky.miller@gmail.com> Co-authored-by: Etienne Dusseault <etienne.dusseault@gmail.com> Co-authored-by: Whymarrh Whitby <whymarrh.whitby@gmail.com> Co-authored-by: Ibrahim Taveras <ibrahimtaveras00@gmail.com> Co-authored-by: Dan J Miller <danjm.com@gmail.com>
rickycodes
pushed a commit
that referenced
this pull request
Jan 31, 2022
* new folder for AccountInfoCard component, remove signature_request.title from message, personal & typed sign components, remove redundant style * SignatureRequest: remove account information from top, proptypes, props, styles, imports * SignatureRequest: change signing message to 'Sign this message?', make bold and larger * remove keyboard aware scroll view * Add AccountInfoCard component to SignatureRequest * AccountInfoCard: implement proper styling * AccountInfoCard: use renderShortAddress, fix styles, use conversionRate to display $ amount * ActionView: add isSigning prop, disables scroll when true, SignatureRequest: pass isSigning=true to ActionView * AccountInfoCard: remove top level view * SignatureRequest: apply styles & layout, add website + arrow icons * Signing components: update styling, rename informationRow --> informationCol * remove message style * TypedSign: put back message style, add messageWrapper style, ensure data fits in box and hides overflow * AccountInfoCard: add snapshot test * update snapshots * styling of 'sign this message' * update snapshot * update snapshot * SignatureRequest: Always render arrow if children coming from TypedSign * SignatureRequest: change to regular component with state to show expanded message content, wrap touchableWithoutFeedback around the message children and move rendering to renderActionViewChildren, tapping the message currently does nothing * fix dapp icon style, fix render inf loop * remove textwrap for typed sign. Now renders properly for V1, V3 & V4 * AccountInfoCard: fix paddings, identicon style, use widths instead of flex, float address to left, fix font weights * TypedSign: use width instead of flex * SignatureRequest: remove website icon wrapper, fix arrow positioning, remove assetLogo style * temp disable warning to match style, ensure message fits within box * PersonalSign: create message wrapper, ensure message fits within box * Message & Personal Sign: use ellipses mode for text wrapper, drop messageWrapper styles * SignatureRequest: remove shouldRenderArrow, add shouldRenderArrow prop * MessageSign: change to stateful component, add renderArrow state, decides if should render arrow upon text component layout, then adjusts the text accordingly * PersonalSign: change to stateful component, add renderArrow state, decides if should render arrow upon text component layout, then adjusts the text accordingly * TypedSign: shouldRenderArrow always passes down as true. Will never be a situation where an entire typed message fits in the box * SignatureRequest: change back to pure component, change handleMessageTap into prop * change handleMessageTap to toggleExpandedMessage * TypedSign: implement message expansion and retraction * modify message, add message_from * new ExpandedMessage component, rendered by typed, personal & regular message components * TypedSign: use ExpandedMessage component * ExpandedMessage: test * SignatureRequest: add renderArrow prop, make box not expandable if false * MessageSign: implement message expanding and retracting * PersonalSign: implement message expand & retract * ExpandedMessage: add mock fns, update all snapshots * ActionView: get rid of top border * new button styles * signing components: ensure a top left and right rounded border * change Cancel & Sign to lowercase * snapshot update * adjust style for android * use unique button style for signing components, fix percentage in stylesheet * change isSigning prop to noScroll * snapshot update * update more snapshots * Signing components: revert to pure component * AccountInfoCard: use weiToFiat & hexToBn helpers to display fiat value, add currentCurrency prop * Signing components: shift renderRootView() contents into render() * update snapshot * AccountInfoCard, SignatureRequest: fix paddings per design * AccountInfoCard: remove bottom margin * TransactionHeader: remove margins, use padding * MessageSign: larger min height, showWarning prop * WarningMessage: use flexstart alignment for bell icon * locales: change eth_sign_warning * WarningMessage: add object as secondary prop type for warning message * SignatureRequest: use WarningMessage component, fix paddings, use renderWarning as prop for WarningMessage * snapshot update yo * SignatureRequest: move AccountInfoCard ontop of message children * snapshot update * AccountInfoCard: remove width * ActionView: remove no scroll - small devices * Signing Components: remove root style, move to SignatureRequest * SignatureRequest: remove style redundancies, add in root style * SignatureRequest: fix height of modal based on screen height * ExpandedMessage: fix styling, move scrollview to signing components renderMessage * ExpandedMessage: Put the scroll back * PersonalSign: remove expandedMessage text wrapper * SignatureRequest: fix up styling, add more overhead (reduced from signing components) * Signing components: reduce view hierarchy, move to SignatureRequest * Locales: add Read more to signature_request * AccountInfoCard: add operation prop, if operation is signing, only display the account name and address * TypedSign: add shouldTruncateMessage & truncateMessage in state * PersonalSign: remove console log * Signing components: change renderArrow to truncateMessage * Nav/Main: add showExpandedMessage to state, add toggleExpandedMessage, configure expanded signing modal to go back on android back button press, pass down props to signing components * Signing Components: move showExpandedMessage out of state, move out toggleExpandedMessage * TypedSign: create different messageWrapper height for iOS & Android, fix text clipping mid-line * ExpandedMessage: fix scrollview * snapshot update * AccountInfoCard: use getTicker * Signing components: change margin bottom from 5 to 4 * Device: new getMediumDevice, SignatureRequest: use getMediumDevice * ActionView, styledButtonStyles: add cancel button style for signing components * snapshot update * SignatureRequest: fix the domain logo not being a circle * update snapshot
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Implement new signature request screen designs
Checklist
AccountInfoCardcomponentIssue
Resolves #1488