Adding network status section on edit gas fee modal #12704
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
mcmire
reviewed
Nov 15, 2021
ui/components/app/edit-gas-fee-popover/network-status/network-status.js
Outdated
Show resolved
Hide resolved
ui/components/app/edit-gas-fee-popover/network-status/status-slider/status-slider.js
Outdated
Show resolved
Hide resolved
ui/components/app/edit-gas-fee-popover/network-status/status-slider/status-slider.js
Outdated
Show resolved
Hide resolved
ui/components/app/edit-gas-fee-popover/network-status/index.scss
Outdated
Show resolved
Hide resolved
mcmire
reviewed
Nov 15, 2021
ui/components/app/edit-gas-fee-popover/network-status/status-slider/status-slider.js
Outdated
Show resolved
Hide resolved
Builds ready [a968d77]Page Load Metrics (779 ± 23 ms)
highlights:storybook
|
mcmire
reviewed
Nov 16, 2021
| <div className="network-status__info-field"> | ||
| <span className="network-status__info-field-data"> | ||
| {gasFeeEstimates?.estimatedBaseFee && | ||
| `${gasFeeEstimates?.estimatedBaseFee} GWEI`} |
There was a problem hiding this comment.
Should this be getting converted to GWEI? I am seeing 0.000000009 GWEI locally. Also according to the designs should we completely round this number to prevent the number from wrapping?
There was a problem hiding this comment.
Yep I also see that locally for test network, but as I understand gasFeeEstimates?.estimatedBaseFee is already in GWEI, thus I guess it is only a test network thing.
There was a problem hiding this comment.
the values looks odd to you ?
in test networks I noticed it varies from .00000009 to something like 213.2342353, not sure what the reason is
There was a problem hiding this comment.
Oh I see. I just wouldn't expect gas fees to be lower than 1 GWEI at all, but if it is sometimes higher then I guess it's a weird quirk of the test net.
Beyond that I think the thing that I'm most worried here is that what happens if the number is more than two decimal places. I wouldn't want the UI to jump in that case. Like, I'm wondering if we do something like:
- If the estimated base fee is < 0.01, show
< 0.01 GWEI. - If the estimated base fee is >= 0.01, show
${baseFee} GWEIwhere the base fee is rounded to 2 decimal places.
If you don't want to fix this now that's fine, we can do another pass, just throwing that out there.
There was a problem hiding this comment.
Good idea, let me work on this.
ui/components/app/edit-gas-fee-popover/network-status/index.scss
Outdated
Show resolved
Hide resolved
ui/components/app/edit-gas-fee-popover/network-status/status-slider/status-slider.js
Outdated
Show resolved
Hide resolved
Builds ready [0ae68a9]Page Load Metrics (734 ± 16 ms)
highlights:storybook
|
Builds ready [505197b]Page Load Metrics (840 ± 56 ms)
highlights:storybook
|
mcmire
previously approved these changes
Nov 18, 2021
ui/components/app/edit-gas-fee-popover/network-status/status-slider/status-slider.js
Outdated
Show resolved
Hide resolved
| <div className="network-status__info-field"> | ||
| <span className="network-status__info-field-data"> | ||
| {gasFeeEstimates?.estimatedBaseFee && | ||
| `${gasFeeEstimates?.estimatedBaseFee} GWEI`} |
jpuri
force-pushed
the
EIP1559_V2_12452
branch
from
505197b
to
9374604
Compare
jpuri
force-pushed
the
EIP1559_V2_12452
branch
from
9374604
to
0cdf4af
Compare
Builds ready [ef43fde]Page Load Metrics (721 ± 11 ms)
highlights:storybook
|
Builds ready [b221dc7]Page Load Metrics (717 ± 15 ms)
highlights:storybook
|
|
@mcmire , @darkwing , @NiranjanaBinoy : PR is updated and ready for re-review. |
darkwing
previously approved these changes
Nov 19, 2021
… EIP1559_V2_12452
…sion into EIP1559_V2_12452
Done 👍 , I hope we soon move to typescript to fix these issues. Thx for the feedback |
Builds ready [9f21228]Page Load Metrics (741 ± 24 ms)
highlights:storybook
|
… EIP1559_V2_12452
Builds ready [556e6dd]Page Load Metrics (722 ± 19 ms)
highlights:storybook
|
mcmire
reviewed
Nov 22, 2021
| <div className="network-status__info"> | ||
| <div className="network-status__info__field"> | ||
| <span className="network-status__info__field-data"> | ||
| {estBaseFee >= 0.01 && `${estBaseFee} GWEI`} |
There was a problem hiding this comment.
It looks like based on the code above that the only reason that estBaseFee could be less than 0.01 is if gasFeeEstimates is null-ish or gasFeeEstimates.estimateBaseFee is null-ish. Assuming that is the case, that fact isn't entirely obvious based on this line alone. What if estBaseFee defaulted to null and then you said estBaseFee !== null && ${estBaseFee} GWEI` here?
There was a problem hiding this comment.
That default was actually inspired from this feedback: #12704 (comment)
| if (gasFeeEstimates?.estimatedBaseFee) { | ||
| // estimatedBaseFee is not likely to be below 1, value .01 is used as test networks sometimes | ||
| // show have small values for it and more decimal places may cause UI to look broken. | ||
| estBaseFee = parseFloat(gasFeeEstimates?.estimatedBaseFee); |
There was a problem hiding this comment.
I think we have to be careful here, because ether can't be represented properly using floats (for instance, 15 WEI is 15 * 10**18, which gets corrupted into 1.5000000000000002e-16). I think the best thing here might be to run this through BigNumber. I feel like you might have run across this code already but I think you can do that by using toBigNumber.dec() from shared/modules/conversion.utils (unless there's another way you know to do this). Of course if this is already a BigNumber then you're good to go! But once you have a BigNumber you can call toFixed(2) on that to format it (while keeping it as a BigNumber).
mcmire
reviewed
Nov 22, 2021
ui/components/app/edit-gas-fee-popover/network-status/network-status.js
Outdated
Show resolved
Hide resolved
darkwing
previously approved these changes
Nov 22, 2021
Builds ready [b91c9e3]Page Load Metrics (775 ± 15 ms)
highlights:storybook
|
mcmire
approved these changes
Nov 23, 2021
darkwing
approved these changes
Nov 23, 2021
adonesky1
reviewed
Nov 23, 2021
| color={COLORS.UI4} | ||
| fontSize="12px" | ||
| > | ||
| <I18nValue |
There was a problem hiding this comment.
Not a blocker, just curious, what was the impetus for creating this component over using the useI18nContext hook?
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #12452
Explanation: Adding network status section on edit gas fee modal.