pelmered / filament-money-field

Money field powered by Money PHP
MIT License
54 stars 20 forks source link

Fix to MoneyInput #11

Closed npbreland closed 7 months ago

npbreland commented 7 months ago

First, thanks for making this plugin. The native handling for money in Filament is lacking! Maybe this plugin will be pulled into Filament core one day? :smile:

I was having a similar issue to #9. As stated in the requirements for this plugin, I am storing my money as an integer value of minor units, and the MoneyColumn looked great, but the MoneyInput was not showing the decimal value correctly, even with the mask. For example, the integer value 400023 was becoming 400,023 instead of 4000.23.

So on the input field hydration, essentially copied what you did for MoneyColumn. However I also created a new static method on the MoneyFormatter to output the intl. decimal format instead (i.e. without the currency symbol), so it will work with and without the mask applied. I added some tests for this new method. Oh, and in the tests, I changed the metadata in the comments to attributes to avoid deprecation notices (apparently support for metadata in the comments will be removed in PHPUnit 12).

Also, I added minValue and maxValue overrides that use the MoneyFormatter to parse the decimal input before comparing with the passed-in max and min values. The base minValue and maxValue methods that are built-in to numeric inputs in Filament were not working because validation seems to occur before dehydration, where the input gets parsed.

I tested the MoneyInput with GBP, USD, and SEK, with and without the mask, and it seemed to work well. Please let me know if I've missed something, and feel free to make edits if needed.

pelmered commented 7 months ago

Thank you! Yes, that was one of the first things I noticed when I started using Filament.

I just skimmed through the PR quickly now, and it looks good. I need to take a closer look, test and make a new release. Hopefully I can do that this weekend. I also want to thank you for the explanation of the PR as well. That was helpful.

npbreland commented 7 months ago

Thanks, Peter! And I'd be happy to help add some tests for the components themselves in the near future.

16 Feb 2024, 08:18 by @.***:

Thank you! Yes, that was one of the first things I noticed when I started using Filament.

I just skimmed through the PR quickly now, and it looks good. I need to take a closer look, test and make a new release. Hopefully I can do that this weekend.

— Reply to this email directly, > view it on GitHub https://github.com/pelmered/filament-money-field/pull/11#issuecomment-1947939417> , or > unsubscribe https://github.com/notifications/unsubscribe-auth/AIICCZZAUW3MLN6BUOQESZ3YT4I6VAVCNFSM6AAAAABDIDB73KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNBXHEZTSNBRG4> . You are receiving this because you authored the thread.> Message ID: > <pelmered/filament-money-field/pull/11/c1947939417> @> github> .> com>

pelmered commented 7 months ago

@npbreland I made a pre-release with this included if you would like to check it out. I would like to receive some feedback or do some more testing before full release.

npbreland commented 7 months ago

@npbreland I made a pre-release with this included if you would like to check it out. I would like to receive some feedback or do some more testing before full release.

@pelmered Sounds good to me. I may have a little time this evening to work on some feature tests for the form input and table column. Looks like Filament has some documentation:

Testing forms Testing tables

npbreland commented 7 months ago

@pelmered I made a start on creating some feature tests this evening, but I'll need more time. I'm somewhat new to Livewire, so this a good chance to learn a bit. I'll keep you updated.

jinbatsu commented 7 months ago

Hello @npbreland , thanks for the fix version 1.2.0.

Already try USD, IDR and EUR, seems okay and it work as expected.

But it breaks some of the currency, like : UEA Dirham => currency: AED, locale: ar_AE. Saudi Riyal => currency: SAR, locale: ar_SA. Example: It display blank, the value in db is 1000

npbreland commented 7 months ago

Hello @npbreland , thanks for the fix version 1.2.0.

Already try USD, IDR and EUR, seems okay and it work as expected.

But it breaks some of the currency, like UEA Dirham => currency: AED, locale: ar_AE. Saudi Riyal => currency: SAR, locale: ar_SA.

No problem @jinbatsu! Thanks for the report. I think the tests only cover SEK and USD at moment, so I definitely want to add some more and figure out how it can support as many currencies as possible. Could you tell me which components you're seeing this in: the table column, form field, etc.?

jinbatsu commented 7 months ago

It is on form field, in Edit and View, I'm not using Infolist, just a form. Edit: And I'm using mask.

npbreland commented 7 months ago

I tried it out in a project of mine with setting the env vars as so:

MONEY_DEFAULT_LOCALE=ar_AE
MONEY_DEFAULT_CURRENCY=AED
MONEY_USE_INPUT_MASK=true

And the input looked good to me: image

However for SAR, the input mask wasn't working, so I had to turn that off

MONEY_DEFAULT_LOCALE=ar_SA
MONEY_DEFAULT_CURRENCY=SAR
MONEY_USE_INPUT_MASK=false

image

Could you try taking off the input mask for SAR? And let me know if the above settings work for you for AED. If you're still seeing an issue, feel free to open an issue so we can better track it. And maybe add a screenshot or better yet, minimal repo we can try out? Filament provides a starter template for minimal repos: https://filament-issue.unitedbycode.com/

pelmered commented 7 months ago

Thanks for reporting @jinbatsu and thanks for helping out @npbreland!

Yes, the mask feature is a bit experimental at the moment as stated in the readme and it is probably the problem here like @npbreland said. I have mostly just tested this with common currencies like USD, EUR etc and SEK as I'm from Sweden. It would be great if we could add some more tests with formatting for more currencies. However, I guess the mask could be a bit tricky to test as a lot of that happens in the front end.

The locale ar_SA is right-to-left, right? That might be the problem here.

jinbatsu commented 7 months ago

@npbreland, I already create sample repo. https://github.com/jinbatsu/sample-filament-money-input

And here is the screenshot: Mask: False 2024-03-01_21-08-18

Mask: True 2024-03-01_21-09-12

@pelmered, thanks for this great plugin, yes it seems the problem with mask and it is right-to-left.

jinbatsu commented 7 months ago

If I'm using Version 1.1.0 composer require pelmered/filament-money-field:1.1.0

It is worked with mask, the display using standar numeric, not arabic, and it is left-to-right.

Screenshot: 2024-03-01_21-17-51