-
Notifications
You must be signed in to change notification settings - Fork 334
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
[DO NOT MERGE] Internationalisation support #2614
Conversation
I had a good conversation with @querkmachine about this the other day. Noting the gist of it here in case it's useful/interesting for others. I asked about whether she'd considered using gettext style calling, where the key is the string in English. She said she had, but decided against it, as she pointed out that if we changed the text for a component, that would force a team with a translation to update that translation, which seems like an unnecessary pain for users, which I agree with. That does though tangentially raise interesting questions about how we encourage conformity; are there ever cases where we will want to make a 'breaking change' to our content, to force teams with translations to update theirs? Of course, there is the wider point that if every team is doing their own translation, then we can expect their to be much consistency anyway. Anyway, I think after discussing it, how @querkmachine has done it using a lookup table with simple keys is in fact the right way to go. I've also been thinking of ways to reduce the need to pass around configuration, but I need to do some experiments with that first... |
It's been suggested that this version of the i18n code (or something similar to it) could be done in an IE-compatible v4 release. On this basis, I've made some minor changes and added polyfills to enable functionality back to IE8. After we stop shipping JavaScript to IE, we can then do a revision to remove polyfills and most of the pluralisation code, as we could then start to leverage the Internationalization API's PluralRules instead. |
Based on a comment @lfdebrux made during a demo, I've tried modifying the function to use a singleton pattern. In this case, the 'default' I18n instance (which is usually the one created within I've kept the ability to manually pass I18n instances into components on the assumption that a service might want to customise specific uses of a component differently to other uses of the same component. |
I've spent some time making the code a little more robust and working on some of the "Improvements that could still be made" bits. A component neglecting to give a translation string a key, or giving it a key that is falsy in JavaScript lingo, will now output an error in the console.
The
Translation strings are now checked if they have placeholders before being passed through I've removed the ability to customise placeholder separators.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking feedback:
- Should we expand the public API? Recommendation: keep it private until a) there is evidenced user need b) it's been in production for a while
- Is a consistent API for individually initialized components worthwhile?
Feel free to disregard any and all feedback as I am a community member.
@vanitabarrett and I had a discussion about how this work will be covered by automated tests. Currently tests are ran against the review app, however the app currently uses the same global Instead, we will unit test the internationalisation JavaScript independently from the review app (as per Vanita's spike, #2611), and provide an example page in the app for visual checks and manual tests. |
d15f998
to
6ec3a33
Compare
The PR has been rebased to include the Nunjucks localisation work from #2648. An example of localised components in action (including localisation performed both via Nunjucks and via JavaScript) can be found here: https://govuk-frontend-pr-2614.herokuapp.com/full-page-examples/localisation |
Something to consider, potentially before we do a pre-release, is the actual naming of the JavaScript localisation keys. I've tried to be descriptive about the purpose of each key when naming them, however this has resulted in some key names being fairly verbose. The list of JS keys currently:
It feels like these could be shortened, for example Likewise, the word 'limit' in the Thoughts? |
</h2> | ||
<nav role="navigation" aria-labelledby="subsection-title"> | ||
<ul class="govuk-list govuk-!-font-size-16"> | ||
<li class="gem-c-related-navigation__link"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think these gem-c classes have snuck in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied this page from one of the other full-page examples, though I'm happy to remove given it's pretty much decorative here.
- name: contentLicence | ||
type: string | ||
required: false | ||
description: HTML or text to replace the default Open Government Licence notice with. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it potentially a slight issue that we split HTML and text into their own properties on other fields, but not here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured splitting it up might not be necessary as it's fairly likely to be HTML. There are other parameters that aren't split (like visuallyHiddenText
), though that does include 'text' in the name.
Would it be better named as contentLicenceHtml
or do you think it's better to split it up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copyright
is a more awkward example of this, I think.
The coat of arms is set as the background image of the crown copyright link. As such, a user cannot use HTML to set a custom link unless hideCrownCopyrightArms: true
(which removes the link and thus the arms), in which case they can. The expected value of copyright
basically switches between being HTML or plain text depending on that context.
- name: hideCrownCopyrightArms | ||
type: boolean | ||
required: false | ||
description: Removes the Crown Copyright notice and royal coat of arms. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to clarify what will be required and suggested in this case. Does the Design System/GDS care if users don't add a new copyright/license to replace these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how necessary that is, might be a question for policy?
For public-facing GOV.UK services, the default (or a translation of the default) is expected, I imagine.
For internal/caseworking systems, they're more likely to remove or change the content licence and potentially remove the coat of arms for space purposes.
For third-parties that use Frontend as a boilerplate, they'll probably remove or replace all of it.
src/govuk/i18n.js
Outdated
* ALL properties passed to `options` will be exposed as options for string | ||
* interpolation, but only `count` and `fallback` initiate special functionality. | ||
* | ||
* TODO: Do we want to put string interpolation options in their own object (e.g. `options.data`) to avoid potential conflict in future? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Useful from the perspective of users in terms of us not introducing breaking changes in the future, though p'raps a bit more mental load for users to take on right now. I wouldn't be against this, but I don't feel strongly that it MUST happen.
I've made a fork of this work based on a suggestion made by @36degrees, for consideration as an alternative approach. You can compare it against this PR here: spike-i18n-support...spike-i18n-support-alt ChangesThe end result is pretty much identical, however the way that localisation strings are configured and scoped is different. Instead of strings being defined within a - { i18n: { accordion: { translations: { show_all_sections: "Show all sections" } } } }
+ { accordion: { i18n: { translations: { show_all_sections: "Show all sections" } } } } Localisations are now part of a component's own configuration, along with any other configuration options. This allows for future configuration options to be nested within the same component object, or for the wrapping object to be discarded entirely when it's unnecessary (for example, when passing a configuration directly into a component). new Accordion($element, {
remember_state: false,
i18n: {
- accordion: {
- translations: {
- show_all_sections: "Show all sections"
- }
- }
+ translations: {
+ show_all_sections: "Show all sections"
+ }
}
}) Other changes
What didn't changeOllie's original proposal was to (roughly) reduce the I18n class to essentially be helper functions that don't need an instantiated instance at all. In this situation, However, the locale information used for pluralisation is currently created on instantiation. Without this, locale information would need to be passed as an option each time I initially made a 'halfway' where I18n was instantiated solely for locale information to be created and string selection was otherwise handled within the component, however this seemed more convoluted than necessary—if we're already creating an I18n instance and passing configuration into it, you might as send the The process of selecting strings within components became much more verbose when not using named keys, too. // Previously, this would suffice
- this.i18n.t('show_all_sections')
// You'd now need to pass in the key relative to the current component and always pass in locale info
+ this.i18n.t(this.options.i18n.translations.show_all_sections, { locale: this.options.i18n.locale }) Ultimately I didn't continue with this approach, as it made implementation within components more complicated for seemingly little additional benefit. ComparisonsUsing `initAll` to pass translation strings to multiple components at onceOld: <script>
window.GOVUKFrontend.initAll({
i18n: {
locale: "cy",
translations: {
accordion: {
show_all_sections: "Dangos pob adran",
hide_all_sections: "Cuddiwch bob adran",
show_this_section: "Dangoswch yr adran hon",
hide_this_section: "Cuddio'r adran hon"
},
character_count: {
characters_over_limit_zero: "Rydych chi %{count} llythyrau drosodd",
characters_over_limit_one: "Rydych chi %{count} llythyr drosodd",
characters_over_limit_two: "Rydych chi %{count} lythyren drosodd",
characters_over_limit_few: "Rydych chi %{count} llythyren drosodd",
characters_over_limit_many: "Rydych chi %{count} llythyren drosodd",
characters_over_limit_other: "Rydych chi %{count} o lythyrau drosodd",
characters_under_limit_zero: "Mae gennych %{count} llythyren ar ôl",
characters_under_limit_one: "Mae gennych %{count} llythyr ar ôl",
characters_under_limit_two: "Mae gennych %{count} lythyren ar ôl",
characters_under_limit_few: "Mae gennych %{count} llythyren ar ôl",
characters_under_limit_many: "Mae gennych %{count} llythyr ar ôl",
characters_under_limit_other: "Mae gennych %{count} llythyren ar ôl"
}
}
}
})
</script> New: <script>
window.GOVUKFrontend.initAll({
accordion: {
i18n: {
locale: "cy",
translations: {
show_all_sections: "Dangos pob adran",
hide_all_sections: "Cuddiwch bob adran",
show_this_section: "Dangoswch yr adran hon",
hide_this_section: "Cuddio'r adran hon"
}
}
},
character_count: {
i18n: {
locale: "cy",
translations: {
characters_over_limit_zero: "Rydych chi %{count} llythyrau drosodd",
characters_over_limit_one: "Rydych chi %{count} llythyr drosodd",
characters_over_limit_two: "Rydych chi %{count} lythyren drosodd",
characters_over_limit_few: "Rydych chi %{count} llythyren drosodd",
characters_over_limit_many: "Rydych chi %{count} llythyren drosodd",
characters_over_limit_other: "Rydych chi %{count} o lythyrau drosodd",
characters_under_limit_zero: "Mae gennych %{count} llythyren ar ôl",
characters_under_limit_one: "Mae gennych %{count} llythyr ar ôl",
characters_under_limit_two: "Mae gennych %{count} lythyren ar ôl",
characters_under_limit_few: "Mae gennych %{count} llythyren ar ôl",
characters_under_limit_many: "Mae gennych %{count} llythyr ar ôl",
characters_under_limit_other: "Mae gennych %{count} llythyren ar ôl"
}
}
}
})
</script> Passing translation strings when individually instantiating a componentOld: <script>
new window.GOVUKFrontend.Accordion($element, {
i18n: {
locale: "cy",
translations: {
accordion: {
show_all_sections: "Dangos pob adran",
hide_all_sections: "Cuddiwch bob adran",
show_this_section: "Dangoswch yr adran hon",
hide_this_section: "Cuddio'r adran hon"
}
}
}
}).init()
</script> New: <script>
new window.GOVUKFrontend.Accordion($element, {
i18n: {
locale: "cy",
translations: {
show_all_sections: "Dangos pob adran",
hide_all_sections: "Cuddiwch bob adran",
show_this_section: "Dangoswch yr adran hon",
hide_this_section: "Cuddio'r adran hon"
}
}
}).init()
</script> |
Adds Nunjucks parameters for customising the content licence and copyright notices in the footer component, and a review app example of this functionality in action. The hardcoded English language versions are still used if the parameters aren't used. I also noticed that the "with custom meta" example was present twice, so I've removed one of them.
Delicately shifts the I18n function to use a singleton pattern. This allows the use of the same instance of I18n across components, without having to explicitly pass though the I18n instance, by using the static I18n.getInstance() method. Currently only the first I18n instance is saved and retrievable by getInstance, as it's assumed this is the 'default' I18n instance and that any subsequent I18n instances are for situations where the default is being overridden (e.g. if a single instance of a component requires different UI labels to other instances of the other components).
If the lookup key specified within a component is falsy (such as being an empty string, null, undefined, or a few other conditions), i18n.js will output an error in the page's console. This will not stop execution of the script, which will instead try to use the fallback string or otherwise output "UNDEFINED".
Adds check to verify that the count is something JavaScript understands as a number. If it is, does some manipulation to make sure the number is positive and not a decimal, as our counting logic doesn't support either of these yet.
Removes the config option to change the separators used by placeholder strings. This is due to placeholders also being present in hardcoded fallback strings, which couldn't easily be changed if the separator configuration was changed. As customising separators was already only a nice to have, I've removed the configuration option for now.
Adds a regular expression to check if a string contains a placeholder before putting it through the function to swap out placeholders. If no placeholders, no loop!
- Adds I18n to the list of expected exports - Adds I18n as an exception to 'exported Components have an init function'
Implements @NickColley's suggestions that I18n be a private function, with components instead using the same configuration object pattern as `initAll`. This also allows for future expansion of component configuration via JavaScript without having to define new parameters.
The test previously checked to see if the value of the fallback text was expected, however the naming and position of the test implies the test should be ensuring that the element is visible. Tests for the text of the element are already present in the template.test.js file, too.
Adds a code check for: - If the Intl class is available in the browser - If the Intl class has the PluralRules object - If the browser has plural rules for the current locale If all three are met, the code will now use Intl.PluralRules instead of the hardcoded plural rule maps. Otherwise, the plural rule maps are used.
Adds Intl.NumberFormat detection, using it for `count` values if supported by the browser. Addresses #2568.
4527657
to
0697445
Compare
The alternative approach described in my last comment has now been merged into this branch, as we felt it was a change that was ultimately necessary for our future API plans. I've also rebased from |
Closing this PR as it contains code developed for the internationalisation spike, which we've completed. The branch will remain so we can refer back to it and/or generate pre-releases as needed. I'm going to draft some new issues that break down the work into sizeable chunks we can start working on implementing this with a release in mind. |
A tech spike into internationalisation (i18n) support in GOV.UK Frontend, particularly within our JavaScript files, which are currently hardcoded. This spike specifically explores a bespoke solution for Frontend, which was the conclusion I came to following the research and tech spikes into third-party solutions (internal document).
Specification
The spike was undertaken based on this rough, self-imposed specification:
initAll
).%{key_name}
otherwise, as GOV.UK uses).How it works
This is a rough, step-by-step of how the example implemention in this spike works. The actual i18n.js file is chock full of documentation too. It's worth a read for how individual methods work.
When using
initAll
initAll
accepts a configuration object where child objects are separated by the component name. This object is split up and each component passed the relevant part of the configuration.Currently only two component namespaces are implemented:
accordion
for the Accordion componentcharacter_count
for the Character Count componentEach component's configuration object may contain a nested
i18n
object, which includes information on the locale to use (used for pluralisation rules and number formatting) and a key-value based list of translations (the 'translation map').This configuration object may also include other configuration in future (though that is outside of the scope of this work).
When initialising components separately
Individually initialised components use the same configuration object format as
initAll
, but with the component namespace removed.Continue...
i18n
key in the config object), each component creates a new instance of the I18n class and passes in the i18n object.i18n.mjs
— Reads in the information given.i18n.mjs
— If no locale information is given, it attempts to read this fromhtml[lang]
. If that is null or empty, it falls back to"en"
. This is made available asthis.locale
.i18n.mjs
— The translation map is made available asthis.translations
.t()
with a lookup key from the previously provided translation map and, optionally, an object containing additional placeholder data. The lookup key is used to find the associated translation string, which may be processed, before being returned to the calling component. See examples.Examples
Assume we've instantiated a non-existent example component with the following configuration for the Pirate language:
In the component's code, we pass the
i18n
information through to the I18n class, and assign the resulting instance to a variable we can use.Using
t()
later in the component will result in these outputs. Note how in the last example that the number has been formatted appropriate to the given locale.Currently...
Currently the spike supports:
initAll
.t
) for selecting a lookup key and interpolating dynamic content.It doesn't support:
Improvements that could still be made: