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

Allow editing of gasPrice & gas in Signer #3777

Merged
merged 47 commits into from
Dec 11, 2016
Merged

Allow editing of gasPrice & gas in Signer #3777

merged 47 commits into from
Dec 11, 2016

Conversation

jacogr
Copy link
Contributor

@jacogr jacogr commented Dec 9, 2016 •
edited
Loading

https://youtu.be/nLfi7kUD3o4

@jacogr jacogr added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M7-ui labels Dec 9, 2016
@jacogr jacogr changed the title Jg signer gas Allow editing of gasPrice & gas in Signer Dec 9, 2016
jacogr
jacogr commented Dec 10, 2016
View reviewed changes
js/src/views/Signer/components/SignRequest/signRequest.spec.js
};

describe('views/Signer/components/SignRequest', () => {
it('renders', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Problematic component, smoketest to test fixes.

@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 10, 2016
ngotchac
ngotchac reviewed Dec 11, 2016
View reviewed changes
js/src/views/Signer/components/TransactionMainDetails/transactionMainDetails.js
@@ -14,52 +14,43 @@
// You should have received a copy of the GNU General Public License
// along with Parity. If not, see <http://www.gnu.org/licenses/>.

import MapsLocalGasStation from 'material-ui/svg-icons/maps/local-gas-station';
Copy link
Contributor

Choose a reason for hiding this comment

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

Nothing to do with this PR, but I was thinking about this:
What about have an Icons util somewhere in the UI ? We don't use that many icons, but still whenever I want to find the right one I end up going to design.google.com. There might even be some inconsistencies in the app (using different icons for the same purpose).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually a good point, like it. Also conforms more to our approach of using our components and, as you mentioned, makes sure we have consistency.

js/src/views/Signer/components/TransactionPendingForm/transactionPendingForm.js
@@ -72,22 +71,23 @@ export default class TransactionPendingForm extends Component {
if (!isRejectOpen) {
html = <span>reject transaction</span>;
} else {
html = <span><BackIcon />I've changed my mind</span>;
html = <span><BackIcon />{ "I've changed my mind" }</span>;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@ngotchac
Copy link
Contributor

ngotchac commented Dec 11, 2016 •
edited
Loading

Looks good, finally got that in there ! :)

A few things though:

  • Failed prop type: The prop "gasLimit" is marked as required in "RequestPending", but its value is "undefined" on signer load
  • Error: gte() not a number: undefined in GasPriceEditor.__setGas when modifying provided gas
  • Invalid prop "value" supplied to "Input" in GasPriceEditor when modifying gas price
  • The Gas Price chart looks a bit flat (whether too large or not tall enough)

@ngotchac ngotchac added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 11, 2016
@ngotchac
Copy link
Contributor

Also, I got Invalid Params when modifying the gas prices / amount. Should I need a the rebuild Parity ?

@jacogr
Copy link
Contributor Author

jacogr commented Dec 11, 2016

Yes. You need latest Parity - gas was not available via the RPC, it is since yesterday.

@jacogr
Copy link
Contributor Author

jacogr commented Dec 11, 2016

The Props stuff got screwed somewhere in one of my merges. (I had some endless issues losing & conflicting changes each time I swapped branches to this one for some obscure reason.)

@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Dec 11, 2016
@jacogr
Copy link
Contributor Author

jacogr commented Dec 11, 2016

Props re-re-added, fixed minor issue in transfer display

@ngotchac
Copy link
Contributor

Everything is working fine.
Still got this one however:

Warning: Failed prop type: Invalid prop `value` supplied to `Input`.
    in Input (created by GasPriceEditor)
    in GasPriceEditor (created by TransactionPending)

when editing gas price from the slider/graphic selector

@ngotchac ngotchac added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 11, 2016
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 26b5690 on jg-signer-gas into ** on master**.

@jacogr jacogr merged commit 929b6ee into master Dec 11, 2016
@jacogr jacogr deleted the jg-signer-gas branch December 11, 2016 16:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ngotchac ngotchac ngotchac left review comments

Assignees
No one assigned
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jacogr @ngotchac @coveralls