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

Hide gas estimate on non-main network #9189

Merged
merged 13 commits into from
Aug 19, 2020

Conversation

PatrykLucka
Copy link
Contributor

This PR removes advanced gas settings option and three-button gas input (slow, average, fast) and shows the gas fields instead on all non-main networks.

Fixes: #9106

Screenshot 2020-08-12 at 16 49 00

Screenshot 2020-08-12 at 16 48 50

@PatrykLucka PatrykLucka requested a review from a team as a code owner August 12, 2020 14:55
@PatrykLucka PatrykLucka requested a review from rekmarks August 12, 2020 14:55
@rekmarks
Copy link
Member

rekmarks commented Aug 12, 2020

Hey Patryk 👋

To get the e2e tests passing, I think we're going need to ensure that the advanced regular gas controls are can be enabled for non-Mainnet networks in tests, which can be done using the process.env.IN_TEST environment variable.

rekmarks
rekmarks previously approved these changes Aug 13, 2020
Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

LGTM!

@whymarrh
Copy link
Contributor

whymarrh commented Aug 13, 2020

Mind rebasing this on the latest develop? I think the consistent-return lint rule might want a value for the return, we should be able to return null there.

rekmarks
rekmarks previously approved these changes Aug 14, 2020
Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

@PatrykLucka upon review, I'm afraid I've failed to give you good advice on this PR. I'm sorry for the back-and-forth.

We should not set the feature flag in order to implement this behavior. Rather, we should do this:

  • If the network is Mainnet, the advanced gas controls showed be shown instead of the gas estimation only if the advancedInlineGas feature flag is set to true.
  • If the network is not Mainnet, we should show the advanced gas controls, regardless of the state of the advancedInlineGas feature flag.
    • Unless we're in a test, in which case we should show the gas estimate.
      • We should only override the feature flag in tests if we have to. I think it's possible that we can leave it alone, and just make all networks behave like Mainnet in tests.

ui/app/store/actions.js Outdated Show resolved Hide resolved
@PatrykLucka
Copy link
Contributor Author

Makes sense, thanks!

@PatrykLucka PatrykLucka force-pushed the hide-advance-gas-on-non-mainnet branch from 4222c28 to 1f2c57e Compare August 17, 2020 10:20
@PatrykLucka PatrykLucka requested a review from rekmarks August 17, 2020 10:22
ui/index.js Outdated Show resolved Hide resolved
rekmarks
rekmarks previously approved these changes Aug 17, 2020
Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

Almost there! In my local testing, there is only one place remaining where ETH Gas Station data needs to be hidden.

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@Gudahtt Gudahtt merged commit 9a3c559 into MetaMask:develop Aug 19, 2020
Gudahtt pushed a commit that referenced this pull request Aug 19, 2020
* hide advance gas on non mainnet

* hide edit gas button on non mainnet
Gudahtt added a commit that referenced this pull request Aug 19, 2020
The changelog has been updated to include #9189
Gudahtt added a commit that referenced this pull request Aug 19, 2020
* Hide gas estimate on non-main network (#9189)

* Update v8.0.9 changelog
@metamaskbot metamaskbot mentioned this pull request Aug 19, 2020
Gudahtt added a commit that referenced this pull request Aug 19, 2020
* origin/develop: (137 commits)
  Use @metamask/eslint-config@3.1.0 (#9275)
  Standardize scss import practices (#9183)
  Update ESLint shared config to v3 (#9274)
  Add lock icon to default networks (#9269)
  Adds toPrecisionWithoutTrailingZeros utility (#9270)
  Hide gas estimate on non-main network (#9189)
  Move the mascot component to its own directory (#9272)
  Use @metamask/controllers@2.0.5 (#9266)
  Fix padding, alignment of actionable-message; add left aligned story
  Code cleanup and simplification for actionable-message component
  Adds actionable message component and stories
  Fix lint issues (#9265)
  Fix prefer-destructuring issues (#9263)
  colocate confirm-decrypt-message page styles (#9252)
  Tidy up Migrator tests (#9264)
  Adds pulse loader component (#9259)
  Fix import/order issues (#9239)
  Fix radix issues (#9247)
  New info tooltip component (#9180)
  Improve scss naming
  ...
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.

Wrong gas estimate on non-main network
4 participants