getAlby / lightning-browser-extension

The Bitcoin Lightning Browser Extension that brings deep Lightning & Nostr integration to the web. Wallet interface to multiple lightning nodes and key signer for Nostr, Liquid and onchain use.
https://getalby.com/#extension
MIT License
539 stars 194 forks source link

Support fiat input in dual currency field #3089

Open riccardobl opened 8 months ago

riccardobl commented 8 months ago

This PR is an initial effort in dealing with https://github.com/getAlby/lightning-browser-extension/issues/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 of fiatValue .

The onChange event is captured and transformed so that event.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 fields

valueInFiat: number;
formattedValueInFiat: string;
valueInSats: number;
formattedValueInSats: string;

to 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 ( account.currency !== "BTC"), the manual switching is temporarily binded to the onClick event of the "alt currency" field Screenshot from 2024-03-16 15-08-02

Let 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.

reneaaron commented 8 months ago

Wow, thanks for tackling that right away! :100:

Let 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.

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! :muscle:

riccardobl commented 8 months ago

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. Both the prefixed symbol and the "fiat amount" will toggle the main currency when clicked.

The reason to have a prefix symbol for sats inputs instead of the "sats" suffix proposed in https://github.com/getAlby/lightning-browser-extension/issues/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:

Screenshot from 2024-03-23 14-51-58

Screenshot from 2024-03-23 14-52-12

Screenshot from 2024-03-23 14-51-44

Screenshot from 2024-03-23 14-51-52

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.

stackingsaunter commented 7 months ago

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..."

pavanjoshi914 commented 7 months ago

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?

riccardobl commented 7 months ago

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

pavanjoshi914 commented 7 months ago

image why i can't remove the zero from the set budget form? i have to type currently for eg. 0100 to set budget for 100 sats. if I press back space it throws error

pavanjoshi914 commented 6 months ago

@riccardobl have you tested things up? is it ready for another round of review?

riccardobl commented 6 months ago

@riccardobl have you tested things up? is it ready for another round of review?

Yes, it should be ready for review

riccardobl commented 6 months ago

Actually both units are visible image First one being ₿.

The tailing unit proposed in the design, as discussed before, is incredibly complex to implement in code as there is no standard component in html that allows tailing units.

I can make the other two changes.

stackingsaunter commented 5 months ago

Hey, yeah both are visable and this is good, but when I enter the screen with input I see only one:

CleanShot 2024-05-24 at 16 13 00@2x

Only after I click on the unit the other appears:

CleanShot 2024-05-24 at 16 13 43@2x

Same happens on "send" screen. I don't know If this was intended or a bug?

riccardobl commented 5 months ago

Oh ok, it was a bug. Should be fixed now, also i've made the other two changes.

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.)

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?

stackingsaunter commented 5 months ago

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

riccardobl commented 5 months ago

fixed

reneaaron commented 5 months ago

@riccardobl Just tested it and that is an awesome improvement. :100:

Can you take a look at the failing unit tests?

pavanjoshi914 commented 5 months ago

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

riccardobl commented 5 months ago

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

I can't reproduce these two issues, could you please double check and give me more info ?

pavanjoshi914 commented 5 months ago

@riccardobl

  1. in receive tab. when i enter amount and remove all numbers via backspace 0 still lies there and new numbers get entered before 0. i have to manually select whole number to cleanup the field
  2. in site settings. if you enter any amount and backspace 0 never vanishes. even after manually selection and trying to remove it + currency conversion seems broken i entered 0.11111 it automatically converted to 165 and it shows 165 dollars = 165 sats https://github.com/getAlby/lightning-browser-extension/assets/55848322/68012ae6-3688-47e1-b44c-23d19660d004
riccardobl commented 5 months ago

@riccardobl

  1. in receive tab. when i enter amount and remove all numbers via backspace 0 still lies there and new numbers get entered before 0. i have to manually select whole number to cleanup the field
  2. in site settings. if you enter any amount and backspace 0 never vanishes. even after manually selection and trying to remove it + currency conversion seems broken i entered 0.11111 it automatically converted to 165 and it shows 165 dollars = 165 sats https://github.com/getAlby/lightning-browser-extension/assets/55848322/68012ae6-3688-47e1-b44c-23d19660d004

should be fixed by the latest commit

stackingsaunter commented 5 months ago

@pavanjoshi914 @reneaaron could you review this? I think it's good to go

pavanjoshi914 commented 4 months ago

wow! great job! currency field is quite stable now.thanks for your valuable work :rocket:

pavanjoshi914 commented 4 months ago

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

  1. value field in the component is not used anywhere. in value field such values are passed to the dualCurrencyCompontent. along with "inputValue" we also have to handle "value" here
  2. values from presets are always in sats. so somehow we need to handle currency conversion + showing the converted value in dual currency field

image

riccardobl commented 4 months ago

This stems from the modified DualCurrencyField not using the value property internally, instead it uses the inputValue state that is initialized with the value of the value property, so it won't react to changes of value even when passed as a react state. This is all due to the fact that the DualCurrencyField presents itself always as a satoshi denominated input and so it has to perform the conversion internally and it needs to update the internal inputValue. There is also some code in alby that updates the value state passed to the DualCurrencyField with the value coming from the DualCurrencyField onChange callback, so a fix needs also to consider this to avoid a feedback loop.

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.

stackingsaunter commented 2 months ago

Could you review that workaround? @pavanjoshi914 and maybe @reneaaron or @bumi ? This is constantly requested by many users

pavanjoshi914 commented 1 month ago

there are still few of the tests need to be fixed