-
Notifications
You must be signed in to change notification settings - Fork 294
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
Amount Preset inputs #1806
Amount Preset inputs #1806
Conversation
* | ||
* const presetAmountInputs: PresetAmountInputs = ['100', '200', '300']; | ||
*/ | ||
export type PresetAmountInputs = readonly [string, string, string]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does this require 3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alessey We only want to have 3 items listed (so that it looks the way we want it on the UI). We don't want to have less than or more than 3 items on the Ui
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷, that seems like something that would be left up to the consumer, doesn't seem like it'd be hard to handle >3 wrapping. Would this component work if I didn't add the input, but only added preset amounts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alessey Wrapping is not hard to handle. It is a design decision to not wrap the preset input amounts and have 3 items or nothing.
Though I have not tested that way but nothing stops it from working with only preset amounts.
}), | ||
[handleChange, handleFiatChange, handleCryptoChange], | ||
); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems strange to extract since its so tightly coupled to this implementation, how are you planning to reuse it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extracted it since I'm using it to control the amount input from outside of the amount input component (from the preset amounts component)
What changed? Why?
Added amount preset inputs for quick input options
PRODUCT REQUIREMENTS:
Screen.Recording.2025-01-17.at.2.06.06.PM.mov
Notes to reviewers
How has it been tested?