-
Notifications
You must be signed in to change notification settings - Fork 193
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
Support fiat input in dual currency field #3089
base: master
Are you sure you want to change the base?
Conversation
Wow, thanks for tackling that right away! 💯
Generally speaking this is great! There might be some nitpicks around naming & event properties but I am well aware this is just a draft so I'll skip that for now. We'll be able to sort those things out as the PR progresses. So looking forward to finally be able to send 10 USD to a friend without having to lookup rates! 💪 |
I propose to use the bitcoin symbol ₿ as prefix for bitcoin inputs (like for $). The input placeholder will still say "Amount in Satoshis..." so it will be clear that it is asking a bitcoin input denominated in sats. The reason to have a prefix symbol for sats inputs instead of the "sats" suffix proposed in #2511 is to keep implementation simple and consistent, since as noted in the issue, implementing a cross-browser growing input field adds quite a lot of complexity. I think this is overall a pretty good alternative, since it is clear, simple and makes use of the ₿ symbol. I've changed the dual currency field to automatically set the parameterized "Amount in {{currency}}..." placeholder when no placeholder is passed to the component and i've adapted the code to not pass a placeholder whenever DualCurrencyField is used, i've still kept the option to have a custom placeholder for special use cases where it might be needed. Some screenshots: Looking forward to your feedback. Also pinging @stackingsaunter for feedbacks on the design since ux is not my forte. The latest commit contains all the aforementioned changes. |
I am wary of getting rid of "sats" for just ₿ but the way you did it is actually a beautiful compromise (referring to screenshots). We still use sats, ₿ just shows in what currency you create an invoice, and even if that would be confusing, there's fiat amount visable so it shouldn't bother much. Love it! One minor thing that could be changed with this PR: The placeholder copy. I'd change it to "Amount in sats..." |
hey @riccardobl i did some testing. the receive screen is broken that is why tests are failing as well. the right sat amount is not getting reflected to the form change functions in receive screen. it always defaults to 0 can you please try and fix that? |
I think i fixed the issues, it seems to be working as expected now. However i'll need some help to get the unit tests updated or figure out why they fail |
@riccardobl have you tested things up? is it ready for another round of review? |
Yes, it should be ready for review |
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.
Hey @riccardobl thanks for those changes! I have a few requests:
-
Behaviour is a bit inconsistent, because the second unit appears only when you click on fiat unit. I think both units should be visible all the time in the input.
-
If it's possible it would be great If units itself would have hover unit (and change the cursor state to "hand with a finger" one), because right now it's not easily discoverable by users they can click it (also because of point 1.)
Oh ok, it was a bug. Should be fixed now, also i've made the other two changes.
I used hover:text-neutral-400 on both, since they have the same styling and added the pointer cursor, is this is the correct way to adapt the original design? |
For dark mode yes, for light mode use gray-600 |
fixed |
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.
tACK
@riccardobl Just tested it and that is an awesome improvement. 💯 Can you take a look at the failing unit tests? |
There's still and inconsistent behaviour. if i enter 0 and press backspace i can't remove the zero from there. also when entering lower amount via fiat. i can't enter for example i can enter 20 sats but not not 0.01 dollars in the fields alsong regarding the tests. its weird that they fail. if i remove dual currency field from the screens. tests do work. can you check that ? fixed the test for DualCurrency component itself |
I can't reproduce these two issues, could you please double check and give me more info ? |
|
should be fixed by the latest commit |
@pavanjoshi914 @reneaaron could you review this? I think it's good to go |
wow! great job! currency field is quite stable now.thanks for your valuable work 🚀 |
label: string; | ||
hint?: string; | ||
amountExceeded?: boolean; | ||
rangeExceeded?: boolean; | ||
baseToAltRate?: number; | ||
showFiat?: boolean; | ||
onChange?: (e: DualCurrencyFieldChangeEvent) => void; | ||
}; | ||
|
||
export default function DualCurrencyField({ |
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.
Now its a good enough complexity. maybe add comments wherere necessary why such and such thing is used
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.
I've added some comments to the code and removed baseToAltRate since it was dead code from one of the previous iterations 👍
max={max} | ||
min={minValue} | ||
max={maxValue} | ||
step={useFiatAsMain ? "0.01" : "1"} |
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.
what if we keep step value upto 4 decimal places. so that user can enter values for 10 20 sats in USD as well (it has no major use but just to keep it fully flexible)
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.
Done
* master: (25 commits) Translated using Weblate (Arabic) Translated using Weblate (German) feat: proper error messages for lndhub Update @getalby/sdk to version 3.6.0 fix: use method to identify connector type fix: show lnaddress only for active account settings feat: use types feat: add http auth kind feat: fix lnaddress setting to show only to alby account feat: extend kind list, kind types, kind translaions fix: remove keys from translations fix: make prompt copy more clear feat: adapt to dynamic width of the container fix: account menu makeover when account name is larger Update i18next to version 23.11.5 Translated using Weblate (Spanish) Translated using Weblate (German) Translated using Weblate (Portuguese (Brazil)) fix: tests fix: unit tests ...
unfortunately this is working when we input values in the input field itself. but we have this presets but when we pass the value from outside for example this presets. dual currency field does nothing. i noticed few things
|
@@ -46,15 +47,6 @@ function Keysend() { | |||
: +amountSat > (auth?.account?.balance || 0); | |||
const rangeExceeded = +amountSat < amountMin; | |||
|
|||
useEffect(() => { |
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.
after passing value for such sats presets and converting them. if dualCurrencyField returns fiatValue then we don't need this useEffect. else we need to explicitly convert the value via such useEffect so that right fiatValue is shown in successCard
This stems from the modified I believe it is not an easy issue to solve, but i thought to it deeply and i've figured out the workaround in my last commit, it is not the prettiest code, but it should work reliably. |
This PR is an initial effort in dealing with #2511 , in its current state it doesn't tweak the rendering of the input field but takes care only of the underlying changes needed to support the currency switch.
To make the implementation effective and reduce code duplication i've moved the switching and conversion logic under the DualCurrencyFieldChangeEvent component that now has an optional boolean
showFiat
field instead offiatValue
.The
onChange
event is captured and transformed so thatevent.target.value
is always a string with the value in satoshis (no matter which currency is used as input),event.target
is also extended with the following custom fieldsto provide all the transformed values needed throughout the various ui components.
DualCurrencyFieldChangeEvent is set to default to use fiat input whenever the account is using a fiat currency (
![Screenshot from 2024-03-16 15-08-02](https://private-user-images.githubusercontent.com/4943530/313410764-58128b83-951b-4adc-a6f8-18ad8c00957e.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjQyNzQwNjQsIm5iZiI6MTcyNDI3Mzc2NCwicGF0aCI6Ii80OTQzNTMwLzMxMzQxMDc2NC01ODEyOGI4My05NTFiLTRhZGMtYTZmOC0xOGFkOGMwMDk1N2UucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MDgyMSUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDA4MjFUMjA1NjA0WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9M2EzZDUwOGRiMjc1NTFiZWNiYzhmNDViNjljZTZiMjkzNGQ2YzllODhhM2ZkNzZhMGNkODMxNmFhZDk5ZjRmYSZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmYWN0b3JfaWQ9MCZrZXlfaWQ9MCZyZXBvX2lkPTAifQ.MeB3MkfN1j16oFcKHzNDHwdyk6Ax0nKgtfVJmsEdPoA)
account.currency !== "BTC"
), the manual switching is temporarily binded to the onClick event of the "alt currency" fieldLet me know what you think about this draft, the proposed ux design is obviously not there yet, but i think it is better to get the logic sorted out first.