happypixels / laravel-shopr

A developer-friendly e-commerce foundation for your Laravel app
https://laravel-shopr.happypixels.se
MIT License
208 stars 19 forks source link

Make it possible to customise money formatting #12

Closed Reached closed 5 years ago

Reached commented 5 years ago

First of all I want to say that working with laravel-shopr is an absolute delight, great job!

Is your feature request related to a problem? Please describe. Currently you can specify your currency code in the ISO format, for instance, "DKK" (Danish Kr.), however, there is no way to change the formatting of this.

Describe the solution you'd like It would be great to be able to format it as "kr" or whatever is possible with the money library you are using.

This is just an example but in the money package that I currently use in my app, they expose this api in the config, perhaps inspiration could be drawn from there:

'DKK' => [
        'name'                => 'Danish Krone',
        'code'                => 208,
        'precision'           => 0,
        'subunit'             => 100,
        'symbol'              => 'kr',
        'symbol_first'        => false,
        'decimal_mark'        => ',',
        'thousands_separator' => '.',
    ],

What do you think?

mattias-persson commented 5 years ago

@Reached Ah thanks so much, that makes me super happy to hear! ❤️

I think this is a great idea, and I've been meaning to make the money formatting a bit more customizable. I'd love to give as much flexibility as possible without bloating the config file though.

What about providing your own MoneyFormatter-class? Something like this:

'currency' => 'DKK',

// The default formatter which you could switch to your own.
'money_formatter' => Happypixels\Shopr\Money\Formatter::class,

And then in the formatter class we could make it super easy to provide whatever details you want and just skip the ones you don't want to modify:

class CustomFormatter extends Happypixels\Shopr\Money\Formatter
{
    // Define the options you want to change. Either like this, or with single line methods. Not sure which approach is the best.
    public static $symbol = 'kr';
    public static $symbolBefore = '$'; // This would be weird, I know. But possible. ('$213kr')
    public static $decimalMark = ','; 

    // We could even allow formatting the amount without any restrictions.
    // Defining this method on your own formatter would be completely optional, your other settings 
    // would still be used when formatting amounts.
    public function format($amount)
    {
        if($amount > 1000){
            return 'Too much money, can\'t handle it.';
        }

        return $amount.' kr';
    }
}

This would create a setup similar to the Nova-resources. What do you think? Would this cover your needs and feel manageable?

Reached commented 5 years ago

I think that is a great solution, extendable and doesn't bloat the config as you pointed out 👌 !

mattias-persson commented 5 years ago

Awesome! Hopefully I can start working on a PR for it this weekend 👍

Reached commented 5 years ago

Awesome! Let me know if I can help! :)

mattias-persson commented 5 years ago

@Reached Would you be able to test this out before I merge it? I believe I've tested it pretty well but always good to have another pair of eyes on it. Using the feature/custom-money-formatter branch, you can now add your own formatter and enable it using the 'money_formatter'-key in the config as we said. You can either customize 5 different options like this:

<?php

namespace App;

use Happypixels\Shopr\Money\Formatter;

class CustomFormatter extends Formatter
{
    public $symbol = ' kr';
    public $symbolBefore = null; // You don't even need this in your class if you don't want to modify it.
    public $thousandSeparator = '.';
    public $decimalCount = 2;
    public $decimalSeparator = ',';
}

Or format the amount freely using the format-method:

<?php

namespace App;

use Happypixels\Shopr\Money\Formatter;

class CustomFormatter extends Formatter
{
    public function format($amount)
    {
        return $amount.' kr';
    }
}

Let me know if you have any questions!

Reached commented 5 years ago

Yea, will definitely do that, will report back once I get to it 👍

I like the API!

Reached commented 5 years ago

Works great when I just return the amount from the format function, but when using the public properties it seems that setting the decimal count to 0 does not remove the decimals, however setting it to 1 does indeed remove one decimal.

Also it seems that there is no obvious way to move the symbol to after the amount without using the public function format($amount) it seems? :)

mattias-persson commented 5 years ago

Ah, I'll take a look on the decimal issue.

Well, I believe the NumberFormatter class automatically puts the symbol on the "correct side" of the amount, depending on your currency code and app locale. Maybe we should add $symbolAfter, and force that to be after like we do with $symbolBefore. And if you want to let PHP decide which side the symbol should be on, you could just use the $symbol property. Would that make sense?

Thanks for your report!

mattias-persson commented 5 years ago

Actually, how about just a $symbol and a $symbolPosition? Feels like that would make the API a bit less confusing.

mattias-persson commented 5 years ago

Ok @Reached, I fixed the decimal issue, removed $symbolBefore and replaced it with $symbolPosition. Set this to either 'before' or 'after' to modify it. If left blank, PHP will auto-position the symbol depending on your currency code and app locale. Let me know if it's working as you'd expect!

Reached commented 5 years ago

Finally got around to test it, and it seems to work perfectly! Good job :) 👍

mattias-persson commented 5 years ago

@Reached Thanks for taking the time to test it out! It really helps. I just have to solve a few problems that came up when running the test suite in Travis, then I'll merge this 👍

Reached commented 5 years ago

No problem!