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

Fix to MoneyInput #11

Merged
merged 4 commits into from
Feb 18, 2024
Merged

Fix to MoneyInput #11

merged 4 commits into from
Feb 18, 2024

Conversation

npbreland
Copy link
Collaborator

@npbreland npbreland commented Feb 14, 2024

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? 😄

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
Copy link
Owner

pelmered commented Feb 16, 2024

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
Copy link
Collaborator Author

npbreland commented Feb 16, 2024 via email

@pelmered pelmered merged commit 4f20e72 into pelmered:main Feb 18, 2024
12 checks passed
@pelmered
Copy link
Owner

@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
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

@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
Copy link

jinbatsu commented Mar 1, 2024

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
Copy link
Collaborator Author

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
Copy link

jinbatsu commented Mar 1, 2024

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

@npbreland
Copy link
Collaborator Author

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
Copy link
Owner

pelmered commented Mar 1, 2024

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
Copy link

jinbatsu commented Mar 1, 2024

@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
Copy link

jinbatsu commented Mar 1, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants