Power-Components / livewire-powergrid

⚡ PowerGrid generates modern, powerful and easy-to-customize data tables using Laravel Livewire.
https://livewire-powergrid.com
MIT License
1.49k stars 220 forks source link

Add translate function to make and action functions of the Column class #1345

Closed viniciusalvess closed 8 months ago

viniciusalvess commented 8 months ago

⚡ PowerGrid - Pull Request

Welcome and thank you for your interest in contributing to our project!. You must use this template to submit a Pull Request or it will not be accepted.


Motivation

Description

This Pull Request adds the translation function to make and action functions of the Column class. It will auto translate the column titles autmatically in case the application uses i18n.

Related Issue(s): #_____.

Documentation

This PR requires Documentation update?

dansysanalyst commented 8 months ago

Hi @viniciusalvess,

🙏 Thank you for contributing!

While I can see the advantage of auto-translating, I am not sure if his could cause confusion and even a "breaking change" in final user experience in some cases.

For the lack of a better example and total unfamiliarity with different localization files, let me propose a scenario of a table component containing a super-market product list in Brazilian Portuguese.

Produto Light Sem Lactose
foo
bar
baz

These column names mean "Product, Light in calories, Lactose-free".

In Portuguese, the word "light" can be translated as "luz" (as in speed of light) or "leve" (as in light in calories).

Developers will get tons of complaints from their clients if "Light" (in calories) turns into "Luz" 💡 after an update.

Again, I doubt that this specific case will happen, but we might have similar situations in different languages.

All that said, I think the auto translation is a cool feature, but I suggest having a flag controlling it.

By default, $autoTranslateColumnTitles = false, if the user needs localization, all them need to do is change it to true.

This provides a convenient way and saves the repetitive work of adding _() in the methods call.

viniciusalvess commented 8 months ago

@dansysanalyst

That's a good point with low probability but it could happen. I like the idea of the flag to enable the auto translation of the column titles. It could also be a config in the livewire-powergrid.php file with the default value false, what do you think?. It would look something like this:


public static function make(string $title, string $field, string $dataField = ''): self
    {
        return (new static())
            ->title(config('livewire-powergrid.autoTranslateColumnTitles', false) ? __($title) : $title)
            ->field($field, $dataField);
    }

    /**
     * Make a new action
     */
    public static function action(string $title): self
    {
        return (new static())
            ->title(config('livewire-powergrid.autoTranslateColumnTitles', false) ? __($title) : $title)
            ->isAction();
    }
dansysanalyst commented 8 months ago

In my opinion, it would be better to configure it individually, per component. This gives flexibility to the user, letting them decide which component to translate.

Also, with Laravel 11 adopting a slim approach towards configuration files, I am not sure what will be of such files in the future.

When the user creates a new PowerGrid component, them will be able to override the property. The documentation can explain this and might even be on the stubs.

// /app/Livewire/AutoTranslateTable.php

class AutoTranslateTable extends PowerGridComponent
{
    public bool $autoTranslate = true,
    //Will auto translate column titles, and other things we don't know yet.
}

⚠️ This might not be the best implementation. It's just the first thing that came into my head late at night.

I am not sure about the performance impact.

//src/PowerGridComponent.php

class PowerGridComponent extends Component
{
   //...
    public bool $autoTranslate  = false;

   //...
   $this->columns = $this->processColumns();

    public function processColumns(): array
    {
        if ($this->autoTranslate === false) {
            return $this->columns();
        }

        return collect($this->columns())->map(fn(Column $column) => $column->title(__($column->title)))->toArray();
    }

}

@luanfreitasdev what do you think?

cool idea @viniciusalvess 😎

dansysanalyst commented 8 months ago

... actually, I think we could give this responsibility to the Column::class.

Something like:

//src/PowerGridComponent.php

public function processColumns(): array
    {
         return this->autoTranslate === true ? $this->columns()->applyTranslations() : $this->columns();
    }
dansysanalyst commented 8 months ago

In addition, t should also translate the placeholder. It doesn't make sense that the column "Document" is now called "Documento" and the placeholder says "Enter your document".

viniciusalvess commented 8 months ago

@dansysanalyst Thank you for working on this enhancement, I believe this enhancement will be a great improvement for future versions of this package.

For now, I solved the issue on my project. Since all my grids inherits from VinPowerGridComponent, I was able to add a bit of code to translate the headers.

//...
class VinPowerGridComponent extends PowerGridComponent
{
  //...

public function columns(): array
  {
    $cols = $this->getColumns();
    foreach ($cols as $k => $c){
      $c->title = __(Str::title(str_replace('_', ' ', $c->title)));
      $cols[$k] = $c;
    }

    return $cols;
  }

  //...
}
luanfreitasdev commented 8 months ago

Hello! @viniciusalvess.

Thanks for sending the PR. As said before, I believe this could be an improvement or a break if it comes by default. Perhaps setting the autoTranslate via properties would fix this problem.

Since you fixed the problem in your environment, I will close this PR. If you want to open another one that works on these points, we can review it.

Thank you!