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

Remove type=number from the max priority fee field. #12041

Closed
wants to merge 1 commit into from

Conversation

segun
Copy link
Contributor

@segun segun commented Sep 8, 2021

Signed-off-by: Akintayo A. Olusegun akintayo.segun@gmail.com

Fixes: #11876

Explanation:

On the gas override dialog (and really all of the dialogs), type=number is very difficult to interact with as a user.

One major glaring issue is that you can't delete the 0 by hitting the delete key. This results in people typing in 010.

Manual testing steps:

Signed-off-by: Akintayo A. Olusegun <akintayo.segun@gmail.com>
@segun segun requested a review from a team as a code owner September 8, 2021 16:24
@github-actions
Copy link
Contributor

github-actions bot commented Sep 8, 2021

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.

@metamaskbot
Copy link
Collaborator

Builds ready [3c63916]
Page Load Metrics (485 ± 47 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint36865548110048
domContentLoaded35264246510450
load3716494859847
domInteractive35264246510450

@@ -68,7 +68,6 @@ export default function AdvancedGasControls({
}}
value={maxPriorityFee}
detailText={maxPriorityFeeFiat}
numeric
Copy link
Member

Choose a reason for hiding this comment

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

This flag does more than set the type of the input element to number. It also ensures the onChange value is a number, and might do other things as well.

If we're considering removing type=number, we should probably remove it from the NumericInput component.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @Gudahtt here -- I don't believe we want to remove this.

@darkwing
Copy link
Contributor

@segun Should we close this or do you plan on updating?

@mcmire mcmire added the needs-decision Needs a decision from MetaMask in order to proceed. label Jan 6, 2022
@danjm
Copy link
Contributor

danjm commented Jan 31, 2022

Closing this as I think we have a different type of fix

@danjm danjm closed this Jan 31, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jan 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs-decision Needs a decision from MetaMask in order to proceed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove type=number
6 participants