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

Use AdvancedGasInputs in AdvancedTabContent #7186

Merged
merged 2 commits into from
Oct 23, 2019

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Sep 18, 2019

The AdvancedGasInputs component was originally extracted from the AdvancedTabContent component, duplicating much of the rendering logic. They have since evolved separately, with bugs being fixed in one place but not the other.

The inputs and outputs expected weren't exactly the same, as the AdvancedGasInputs component converts the input custom gas price and limit, and it converts them both in the setter methods as well. The GasModalPageContainer had to be adjusted to avoid converting these values multiple times.

A few other changes were made to avoid using variables as parameters to the localization helper function. This is to make it easier to detect when messages are unused.

Closes #6872

@Gudahtt Gudahtt force-pushed the consolidate-advanced-gas-inputs branch 3 times, most recently from 85f800f to e3b2a95 Compare September 19, 2019 13:25
@Gudahtt
Copy link
Member Author

Gudahtt commented Sep 19, 2019

This should be ready to review now. The tests had been failing because a debounce was added to one of the fields that wasn't there before, so an additional pause was required.

The previous gas controls in AdvancedTabContent are fairly similar to AdvancedGasInputs (the latter was derived from the former after all), but there were some style differences. I tried to make it look fairly similar.

Here is the old version:
oldUI

And here is the new version:
newUI

@Gudahtt Gudahtt force-pushed the consolidate-advanced-gas-inputs branch 3 times, most recently from d9463cf to 6b49e5a Compare October 17, 2019 01:48
@Gudahtt
Copy link
Member Author

Gudahtt commented Oct 17, 2019

I had to update this PR to fix tests after a rebase, then I reconsidered an earlier decision I made.

Both components dealt with input debouncing separately, both in less than ideal ways. AdvancedTabContent didn't debounce either field, but it did debounce the check for whether the gas limit field was below the minimum value. So if a less-than-minimum value was set, it would be propogated upwards and could be saved if the user clicked 'Save' quickly enough. After a second delay it would snap back to the minimum value. The AdvancedGasInputs component debounced both fields, but it would replace any gas limit below the minimum with the minimum value. This led to a problem where a brief pause during typing would reset the field to 21000.

At first, the AdvancedGasInputs approach was chosen for the new merged component. I figured that this was the "less bad" of the two, and the problem encountered was an existing issue that hadn't been flagged yet as a problem. Though after reconsidering, I decided to mitigate it now, as introducing this problem in new contexts would basically constitute a regression.

The new behavior now matches the old AdvancedGasInputs behavior, except that it was updated to no longer change the gas limit if it was below the minimum. Instead it displays an error. Parent components were checked to ensure they would detect the error case of the gas limit being set too low, and prevent the form submission in those cases. Of the three parents, one had already dealt with it correctly, one needed to convert the gas limit from hex first, and another needed the gas limit check added.

danjm
danjm previously approved these changes Oct 21, 2019
Copy link
Contributor

@danjm danjm left a comment

Choose a reason for hiding this comment

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

QA'd and reviewed, looks good.

@Gudahtt Gudahtt force-pushed the consolidate-advanced-gas-inputs branch from 6b49e5a to 012d205 Compare October 22, 2019 17:19
The `AdvancedGasInputs` component was originally extracted from the
`AdvancedTabContent` component, duplicating much of the rendering
logic. They have since evolved separately, with bugs being fixed in one
place but not the other.

The inputs and outputs expected weren't exactly the same, as the
`AdvancedGasInputs` component converts the input custom gas price and
limit, and it converts them both in the setter methods as well.
The `GasModalPageContainer` had to be adjusted to avoid converting
these values multiple times.

Both components dealt with input debouncing separately, both in less
than ideal ways. `AdvancedTabContent` didn't debounce either field, but
it did debounce the check for whether the gas limit field was below the
minimum value. So if a less-than-minimum value was set, it would be
propogated upwards and could be saved if the user clicked 'Save'
quickly enough. After a second delay it would snap back to the minimum
value. The `AdvancedGasInputs` component debounced both fields, but
it would replace any gas limit below the minimum with the minimum
value. This led to a problem where a brief pause during typing would
reset the field to 21000.

The `AdvancedGasInputs` approach was chosen, except that it was
updated to no longer change the gas limit if it was below the minimum.
Instead it displays an error. Parent components were checked to ensure
they would detect the error case of the gas limit being set too low,
and prevent the form submission in those cases. Of the three parents,
one had already dealt with it correctly, one needed to convert the
gas limit from hex first, and another needed the gas limit check added.

Closes MetaMask#6872
Empty README files have been removed, and a mistake in the index file
for the send page has been corrected. The Gas Slider component class
name was updated as well; it looks like it was originally created from
`AdvancedTabContent`, so it still had that class name.
@Gudahtt Gudahtt force-pushed the consolidate-advanced-gas-inputs branch from 012d205 to 7db4fea Compare October 23, 2019 03:49
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.

Bug with gas input field
2 participants