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

Allow properties of jsonable attributes of a model to be translatable #21

Open
multiwebinc opened this issue Jan 26, 2022 · 24 comments · May be fixed by #76
Open

Allow properties of jsonable attributes of a model to be translatable #21

multiwebinc opened this issue Jan 26, 2022 · 24 comments · May be fixed by #76

Comments

@multiwebinc
Copy link
Contributor

I think this would be a good idea to allow properties of jsonable attributes to be translatable. This would allow you to translate specific sub-fields of a repeater to be translated. For example:

Model:

class Gallery extends Model
{
    public $implement = ['@RainLab.Translate.Behaviors.TranslatableModel'];

    protected $jsonable = ['images'];

    public $translatable = [
        'images.alt_text',
        'images.description',
    ];
}

Then in the yaml:

tabs:
    fields:
        images:
            type: repeater
            form:
                fields:
                    image:
                        label: Image
                        type: mediafinder
                        mode: image
                        imageHeight: 150
                        imageWidth: 250
                        span: left
                        required: true
                    alt_text:
                        span: right
                    description:
                         type: textarea
                         span: full

If someone could give me some direction on how to even get started, I might be able to get a PR for this.

@LukeTowers
Copy link
Member

@bennothommo any thoughts?

@mjauvin
Copy link
Member

mjauvin commented Jan 26, 2022

If I remember correctly, the current architecture does not really make this possible... but I'd LOVE if this was implemented, it would solve so many problems for my use case.

@bennothommo
Copy link
Member

I'd say it wouldn't be possible simply because of the lack of IDs in the JSON-able data - you'd essentially have to copy the entire JSON data over for each translation and link it to the main record.

In this scenario, it would be structurally better to use a related model with translatable fields.

@multiwebinc
Copy link
Contributor Author

@bennothommo Another option I thought of would be to store the translations right in the JSON. Like having fieldName, then fieldName[en], fieldName[es], fieldName[fr], etc.

@bennothommo
Copy link
Member

@multiwebinc that could work. We'd have to establish (in the docs, perhaps?) that the developer will need to account for this extra data if they go down this route, and use a larger data format column (like say a BIGTEXT column in their database), but it's certainly within the realm of possibility.

@mjauvin
Copy link
Member

mjauvin commented Jan 31, 2022 via email

@multiwebinc
Copy link
Contributor Author

@bennothommo Presumably for columns that contain JSON, people should be using the JSON type (i.e. $table->json() in their migrations). Assuming Laravel actually uses the JSON data type in the DB, this gives a limit of 1GB for MySQL and 255 MB for PostgreSQL. SQLite is around 1GB for TEXT or BLOB. So it's very unlikely these limits would ever be reached. Of course, it wouldn't hurt to add a note to the docs in case people use TEXT. What happens if the value exceeds the column limit? Is it truncated, or does the query fail?

@mjauvin Good point. I can't really think of a truly elegant way to get around that because of the lack of IDs, but perhaps when the translate plugin has been disabled and someone tries to write to a translatable JSON field, present a confirmation dialog saying something like "The field you are editing is translatable, but the Translate plugin has been disabled or removed. Making changes will remove any translated strings." Maybe also recommending creating a backup, re-enabling the plugin, removing the $translatable property of the model, etc. I would assume that most times people disable the plugin it's because they no longer want to use it, so the translation strings are generally not important anyway.

@mjauvin
Copy link
Member

mjauvin commented Jan 31, 2022

@multiwebinc if the plugin is disabled, you wouldn't be able to inform the user about this, unless you have some exception handling code in wintercms core

@multiwebinc
Copy link
Contributor Author

@mjauvin Yeah, I was thinking of putting it in the core.

@multiwebinc
Copy link
Contributor Author

@mjauvin Another idea that just occurred to me is to add something like a backend.plugin.afterRemove hook that will allow plugin developers to display a popup (or plain text message in the CLI) one single time after the plugin is deleted. This could be used to explain what happens to any residual data (such as translations) that are not deleted, any further steps that might be necessary, to display a discount message for people who might want to purchase a pro version of the plugin at a discounted rate, or to display a generic "We're sorry to see you go" type message.

@kubamarkiewicz
Copy link

I'd love to see this feature implemented, that would make using jsonable fields on multilingual websites possible.

@mjauvin
Copy link
Member

mjauvin commented Feb 22, 2024

@multiwebinc @kubamarkiewicz I would appreciate it if you guys could test PR #76

@kubamarkiewicz
Copy link

That's great news, I'll test it!

@mjauvin
Copy link
Member

mjauvin commented Feb 25, 2024

@kubamarkiewicz I added a comment in the PR's description to clone the branch directly in order to test this.

@kubamarkiewicz
Copy link

@mjauvin I made some tests with repeater and nestedform fields and works almost perfectly!

I have found only one problem. When I have repeater field configured as in the example that you have included in Readme.md then when I save translations and reload the form, then when I switch the language in 'Job Title' field, the value does not change.
I works correctly however, with the following configuration:

    public $translatable = [
        'data',
        'data[title]',
    ];
fields:
  data:
    type: repeater
    translationMode: fields
    form:
      fields:
        name:
          label: Name
        title:
          label: Job Title
        phone:
          label: Phone number

@kubamarkiewicz
Copy link

kubamarkiewicz commented Feb 25, 2024

It would be also great if translations for 'blocks' field type could be implemented in a similar way. There is a difference however, because in that case there is no possibility to configure 'translatable' property in the controller and I have suggested adding 'translatable' property in fields definition in yaml: wintercms/wn-blocks-plugin#25

Do you think that would be possible?

@mjauvin
Copy link
Member

mjauvin commented Feb 25, 2024

@kubamarkiewicz I was missing data in the translatable array in my example.

This is needed in order to fetch the right version of the data for the selected locale.

@mjauvin
Copy link
Member

mjauvin commented Feb 25, 2024

It would be also great if translations for 'blocks' field type could be implemented in a similar way. There is a difference however, because in that case there is no possibility to configure 'translatable' property in the controller and I have suggested adding 'translatable' property in fields definition in yaml: wintercms/wn-blocks-plugin#25

Do you think that would be possible?

Once this gets released, we'll look into adding support to the blocks plugin.

@kubamarkiewicz
Copy link

@mjauvin ok, now the example in Readme.md works correctly.

@kubamarkiewicz
Copy link

@mjauvin I made some more in depth testing of repeater field and I have found a problem with reordering items: when there are two items saved in a repeater field, when I reorder them and refresh the page, the fields that are translatable (in this case 'Job title') appear in wrong items:

image

@mjauvin
Copy link
Member

mjauvin commented Feb 26, 2024

Interesting find.

If you can continue those within the PR itself, that would be appreciated.

@kubamarkiewicz
Copy link

Would it be possible to allow alternative syntax to specify translatable fields inside .yaml file?

fields:
  data[contacts]:
    type: repeater (or nestedform)
    translationMode: fields
    form:
      fields:
        name:
          label: Name
          translatable: true
        title:
          label: Job Title
          translatable: true
        phone:
          label: Phone number

The goal is to be able to define jsonable fields entirely in fields.yaml file without necessity to modify the model file.
I have a plugin where .yaml file containing nested form definition is located in theme folder and I would like to allow people to define fields there without touching the model file.

The syntax could be similar to the proposed syntax for translations in blocks plugin: wintercms/wn-blocks-plugin#25

What do you think about it?

@mjauvin
Copy link
Member

mjauvin commented Mar 27, 2024

Would it be possible to allow alternative syntax to specify translatable fields inside .yaml file?

fields:
  data[contacts]:
    type: repeater (or nestedform)
    translationMode: fields
    form:
      fields:
        name:
          label: Name
          translatable: true
        title:
          label: Job Title
          translatable: true
        phone:
          label: Phone number

The goal is to be able to define jsonable fields entirely in fields.yaml file without necessity to modify the model file. I have a plugin where .yaml file containing nested form definition is located in theme folder and I would like to allow people to define fields there without touching the model file.

The syntax could be similar to the proposed syntax for translations in blocks plugin: wintercms/wn-blocks-plugin#25

What do you think about it?

I wanted to do that as well... thanks for confirming this not only makes sense to me.

@kubamarkiewicz
Copy link

@mjauvin definitely it makes sense!

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

Successfully merging a pull request may close this issue.

5 participants