frappe / datatable

The Missing Javascript Datatable for the Web
https://frappe.io/datatable
MIT License
1.02k stars 163 forks source link

feat: translations #145

Closed hrwX closed 2 years ago

hrwX commented 2 years ago
barredterra commented 2 years ago

@hrwX can you add an example how this can be used? How do I get my custom labels to use the original actions?

https://github.com/frappe/datatable/blob/699ef5daa221270578d54bad3d01f5d6b823f212/src/defaults.js#L9-L33

hrwX commented 2 years ago

@hrwX can you add an example how this can be used? How do I get my custom labels to use the original actions?

https://github.com/frappe/datatable/blob/699ef5daa221270578d54bad3d01f5d6b823f212/src/defaults.js#L9-L33

@barredterra

barredterra commented 2 years ago

@hrwX thanks for adding the example, looks good to me.

@netchampfaris what do you think?

netchampfaris commented 2 years ago

I think a better API for translations would be:

let dt = new DataTable({
  ...
  translations: {
    'Sort Ascending': __('Sort Ascending'),
    'Sort Descending': __('Sort Descending'),
  }
})

You provide alternative text for every user-facing text that is rendered in DataTable.

@hrwX @barredterra Thoughts?

barredterra commented 2 years ago

@netchampfaris thanks for your suggestion! Kindly review again whenever you have time. 😊

netchampfaris commented 2 years ago

@hrwX Hey, thanks for the quick update.

However, I think we should extend this for every user-facing text in DataTable. For e.g., "No Data" when there are no rows. Every user-facing text must be wrapped in a function that will then look up translations strings passed as options. This will make sure that user-facing text added in the future are also covered.

barredterra commented 2 years ago

@netchampfaris @hrwX if we pass a function, we also need to pass variables and context to that function. This way datatable and frappes __() function will get somewhat coupled and anybody else using datatable will probably need to write a wrapper function for their own translation method. Also we would need to copy part of it to do variable substitution if no translation function is provided.

Another option would be to use something like http://i18njs.com/ and provide our own translations in this repo to keep it as a standalone solution. There are <10 translatable strings in this repo, so this would't cause much overhead.

hrwX commented 2 years ago

@netchampfaris @hrwX if we pass a function, we also need to pass variables and context to that function. This way datatable and frappes __() function will get somewhat coupled and anybody else using datatable will probably need to write a wrapper function for their own translation method. Also we would need to copy part of it to do variable substitution if no translation function is provided.

Another option would be to use something like http://i18njs.com/ and provide our own translations in this repo to keep it as a standalone solution. There are <10 translatable strings in this repo, so this would't cause much overhead.

@netchampfaris Correct me if I am wrong @barredterra What faris is saying is that write a frappe's _ equivalent function in datatable, and if translations are provided, then just replace it.

hrwX commented 2 years ago

@netchampfaris @barredterra have added i18njs for handling the translations for user-facing strings. Can you guys take a look at it

netchampfaris commented 2 years ago

@barredterra What faris is saying is that write a frappe's _ equivalent function in datatable, and if translations are provided, then just replace it.

Yes, this is what I meant. I would like to avoid adding a new library 😅

barredterra commented 2 years ago

@netchampfaris those libraries are tiny (~200 lines of code). If you prefer we can just copy and paste the code here.

hrwX commented 2 years ago

@barredterra @netchampfaris Was able to implement a translation manager similar to the ones in frappe. Also after implementing frappe like translations, seems like i18njs would have been a bloatware since we would just use it for 4/5 strings.

what do you guys think ?

github-actions[bot] commented 2 years ago

:tada: This PR is included in version 1.16.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: