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

Add togglable advanced gas controls on send and confirm screens #6112

Merged
merged 9 commits into from
Feb 6, 2019

Conversation

danjm
Copy link
Contributor

@danjm danjm commented Feb 5, 2019

Fixes #5931

Demo:

peek 2019-02-05 13-50

cc @whymarrh @cjeria

@danjm danjm requested a review from whymarrh as a code owner February 5, 2019 17:22
@danjm
Copy link
Contributor Author

danjm commented Feb 5, 2019

I still need to update tests, but code should be good to review

@cjeria
Copy link
Contributor

cjeria commented Feb 5, 2019

@danjm Can you add more padding above the "advanced options" button link? Like 16px. It's so tight up against the controls above it and could use more breathing room.

@@ -0,0 +1,147 @@
import React, { Component } from 'react'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this code is somewhat of a subset of code already in metamask-extension/ui/app/components/gas-customization/gas-modal-page-container/advanced-tab-content/advanced-tab-content.component.js

I will probably remove corresponding code from that component in a future task

constructor (props) {
super(props)

this.debouncedGasLimitReset = debounce((dVal) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move these properties out of the constructor and use the property syntax? (And then even remove the empty constructor.)

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 in 0112680d6

@danjm
Copy link
Contributor Author

danjm commented Feb 5, 2019

user experience when confirming from a dapp:

peek 2019-02-05 14-47

@danjm
Copy link
Contributor Author

danjm commented Feb 5, 2019

@cjeria How does this look re: advanced options button?

screenshot from 2019-02-05 14-59-28

@cjeria
Copy link
Contributor

cjeria commented Feb 5, 2019

@danjm Can you move it down like 10px-16px more? Sry, super nitpic i know

@danjm danjm force-pushed the i5931-inline-advanced-gas-inputs branch from 0112680 to 938f9f9 Compare February 5, 2019 19:53
@danjm
Copy link
Contributor Author

danjm commented Feb 5, 2019

@cjeria

screenshot from 2019-02-05 16-22-48

@metamaskbot
Copy link
Collaborator

Builds ready [938f9f9]: mascara, chrome, firefox, edge, opera

@cjeria
Copy link
Contributor

cjeria commented Feb 5, 2019

Great! thx @danjm

@danfinlay danfinlay merged commit 38b91f6 into develop Feb 6, 2019
@whymarrh whymarrh deleted the i5931-inline-advanced-gas-inputs branch February 6, 2019 00:53
@danfinlay danfinlay mentioned this pull request Feb 7, 2019
@bdresser bdresser mentioned this pull request Feb 7, 2019
@tmashuang tmashuang restored the i5931-inline-advanced-gas-inputs branch February 7, 2019 22:12
@danjm danjm deleted the i5931-inline-advanced-gas-inputs branch March 26, 2019 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants