Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Signer method parameter decoding & destination info #3671

Merged
merged 6 commits into from
Dec 3, 2016

Conversation

jacogr
Copy link
Contributor

@jacogr jacogr commented Nov 29, 2016

@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Nov 29, 2016
@jacogr jacogr added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 29, 2016
@jacogr
Copy link
Contributor Author

jacogr commented Nov 29, 2016

Marking WIP until https://github.com/ethcore/parity/pull/3642 is in and changes pulled-in.

# Conflicts:
#	js/src/views/Signer/components/RequestFinished/requestFinished.js
@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Dec 2, 2016
@ngotchac ngotchac added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 2, 2016
@ngotchac
Copy link
Contributor

ngotchac commented Dec 2, 2016

This is a great improvement ! Especially removing that much unused code..

Small grumble : the very first column seems a bit big ; what about something like this :

diff --git a/js/src/views/Signer/components/TransactionMainDetails/TransactionMainDetails.css b/js/src/views/Signer/components/TransactionMainDetails/TransactionMainDetails.css
index 92ed968..3e077f3 100644
--- a/js/src/views/Signer/components/TransactionMainDetails/TransactionMainDetails.css
+++ b/js/src/views/Signer/components/TransactionMainDetails/TransactionMainDetails.css
@@ -19,6 +19,7 @@
 
 .transaction {
   flex: 1;
+  display: flex;
   overflow: auto;
 }
 
@@ -31,8 +32,8 @@
 }
 
 .from {
-  width: 40%;
   vertical-align: top;
+  padding: 0 3em;
 
   img {
     display: inline-block;
@@ -47,7 +48,7 @@
 }
 
 .method {
-  width: 60%;
+  flex: 1;
   vertical-align: top;
   line-height: 1em;
 }

@jacogr
Copy link
Contributor Author

jacogr commented Dec 2, 2016

The issue with your approach is the following -

parity 2016-12-02 15-15-48

The current approach has all aligned in columns, no matter what the content is

@ngotchac
Copy link
Contributor

ngotchac commented Dec 2, 2016

Hmmm... Fair point

@ngotchac ngotchac added A8-looksgood 🦄 Pull request is reviewed well. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Dec 2, 2016
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 85.973% when pulling fbd3738 on jg-signer-decoding into b8b9086 on master.

@gavofyork gavofyork merged commit be7401c into master Dec 3, 2016
@gavofyork gavofyork deleted the jg-signer-decoding branch December 3, 2016 06:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants