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

feat: translations #145

Merged
merged 10 commits into from
Jan 3, 2022
Merged

Conversation

hrwX
Copy link
Contributor

@hrwX hrwX commented Nov 24, 2021

  • Adds translations support to datatable.
  • Language can be passed as parameter while initializing the datatable.
  • Translations can be added in translations.js.
  • Custom translations can be added using customTranslations when initializing the datatable.
  • Handling Pluralization:
    • to translate strings that can have a plural word like row/rows, multiple strings can be defined for counts, ie for a particular count, if a string is defined, it'll be used. eg: if the total row count is 0, the string for key 0 will be using in {0} rows selected the translations and if there exists no key like 10, default key will be used for translation. This way multiple use cases can be handled
    this.translations = {
            en: {
                '{count} rows selected': {
                    0: '{count} rows selected',
                    1: '{count} row selected',
                    default: '{count} rows selected'
                }
            }
        };

@barredterra
Copy link
Contributor

barredterra commented Nov 24, 2021

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

{
label: 'Sort Ascending',
action: function (column) {
this.sortColumn(column.colIndex, 'asc');
}
},
{
label: 'Sort Descending',
action: function (column) {
this.sortColumn(column.colIndex, 'desc');
}
},
{
label: 'Reset sorting',
action: function (column) {
this.sortColumn(column.colIndex, 'none');
}
},
{
label: 'Remove column',
action: function (column) {
this.removeColumn(column.colIndex);
}
}
],

@hrwX
Copy link
Contributor Author

hrwX commented Nov 25, 2021

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

{
label: 'Sort Ascending',
action: function (column) {
this.sortColumn(column.colIndex, 'asc');
}
},
{
label: 'Sort Descending',
action: function (column) {
this.sortColumn(column.colIndex, 'desc');
}
},
{
label: 'Reset sorting',
action: function (column) {
this.sortColumn(column.colIndex, 'none');
}
},
{
label: 'Remove column',
action: function (column) {
this.removeColumn(column.colIndex);
}
}
],

@barredterra

  • Let's say we are initializing the datatable in a script, and we want to override the headerDropdown, we can easily do that using
		let me = this;
		this.datatable = new DataTable(this.$datatable_wrapper[0], {
			columns: [],
			data: [],
			overrideHeaderDropdown: true,
			headerDropdown: [{
				label: __('Sort Ascending'),
				action: function (column) {
					// 'me' is used since this references the DataTable object in here
					me.datatable.sortColumn(column.colIndex, 'asc');
				}
			}]
		});
  • this is just relevant to the frappe framework implementation.
  • Either we can go with this or build a feature for supporting translations in the library itself.

@barredterra
Copy link
Contributor

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

@netchampfaris what do you think?

@netchampfaris
Copy link
Collaborator

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

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

@netchampfaris
Copy link
Collaborator

@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.

src/datatable.js Outdated
setTranslations() {
if (!this.options.translations) return;

this.options.headerDropdown.forEach(header => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method only handles translations for labels in headerDropdown, it should handle all user-facing text. A better approach would be to wrap all user-facing text in a translation function for e.g., _('Sort descending') and let that function do the replacement.

@barredterra
Copy link
Contributor

barredterra commented Dec 6, 2021

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

hrwX commented Dec 7, 2021

@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 hrwX changed the title feat: ability to override header dropdown feat: translations using i18njs Dec 12, 2021
@hrwX hrwX force-pushed the overrideHeaderDropdown branch 4 times, most recently from 61660a2 to 0b09d6c Compare December 12, 2021 18:13
@hrwX
Copy link
Contributor Author

hrwX commented Dec 13, 2021

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

@netchampfaris
Copy link
Collaborator

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

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

@hrwX hrwX changed the title feat: translations using i18njs feat: translations Dec 17, 2021
@hrwX
Copy link
Contributor Author

hrwX commented Dec 17, 2021

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

src/utils.js Outdated Show resolved Hide resolved
@hrwX hrwX force-pushed the overrideHeaderDropdown branch 2 times, most recently from 9ceb0b2 to 3e472c2 Compare December 17, 2021 15:45
'No Data': 'No Data',
'{0} {1} copied': '{0} {1} copied',
'{0} {1} selected': '{0} {1} selected'
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

        de: {
                'Sort Ascending': 'Aufsteigend sortieren',
                'Sort Descending': 'Absteigend sortieren',
                'Reset sorting': 'Sortierung zurücksetzen',
                'Remove column': 'Spalte entfernen',
                'No Data': 'Keine Daten',
                '{0} cells copied': {
                    0: '{0} zellen kopiert',
                    1: '{0} zelle kopiert',
                    default: '{0} zellen kopiert'
                },
                '{0} rows selected': {
                    0: '{0} zeilen ausgewählt',
                    1: '{0} zeile ausgewählt',
                    default: '{0} zeilen ausgewählt'
                }
            }

src/translationmanager.js Outdated Show resolved Hide resolved
src/rowmanager.js Outdated Show resolved Hide resolved
Co-authored-by: Raffael Meyer <14891507+barredterra@users.noreply.github.com>
src/translationmanager.js Outdated Show resolved Hide resolved
src/translations/de.json Outdated Show resolved Hide resolved
src/translations/en.json Outdated Show resolved Hide resolved
src/translationmanager.js Outdated Show resolved Hide resolved
src/translationmanager.js Show resolved Hide resolved
hrwX and others added 7 commits December 20, 2021 11:36
Co-authored-by: Raffael Meyer <14891507+barredterra@users.noreply.github.com>
Co-authored-by: Raffael Meyer <14891507+barredterra@users.noreply.github.com>
Co-authored-by: Raffael Meyer <14891507+barredterra@users.noreply.github.com>
* Create fr.json

* feat: italian translations

* feat: add "fr" and "it" to getTranslationsJSON
@netchampfaris netchampfaris merged commit 6d6602f into frappe:master Jan 3, 2022
@github-actions
Copy link

github-actions bot commented Jan 5, 2022

🎉 This PR is included in version 1.16.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

3 participants