Add macro options to configure CharacterCount translations #2895

romaricpascal · 2022-10-03T16:00:02Z

Lets the CharacterCount macro receive options to set its client-side translations.

To avoid cluttering the options, the way I18n handles pluralisation has been reworked to use an object containing a plural-type to message mapping rather than individual suffixed keys, like proposed by @36degrees below.

As the commit history shows, this involved:

updating the I18n class to use . based suffixes rather than camelCased ones
checking that keys in the config maps were flattened on more than one level deep (which they were, cheers @querkmachine 🙌🏻 )
updating the character count, its test and docs to match the new format
adding a helper macro to generate the internationalisation attributes, to reduce boilerplate inside the templates themselves. This led to creating a separate callMacro test helper for getting the output of a macro (as we're not just rendering components).
passing options from the macro to the HTML

Closes #2701, with the heavy lifting of getting the translations from the data attributes and using them done in #2887

It's worth noting as well that the new option names cause some wrapping in the docs, for which I've opened alphagov/govuk-design-system#2369.

36degrees

I think we need to extend this approach to also allow one, two, few and many plural forms…

At the minute this only supports localising into languages that only use the one or other plural forms – based on our current implementation, it'd be fine for languages that use the Chinese, French or German pluralisation systems but not Arabic, Irish, Russian, Scottish or Welsh (!).

That's a lot of options we'll need to add 😱

It might be a bit late to suggest this, but I'm also wondering if we should consider namespacing the different forms together in both the Nunjucks and the config object so that you can do something like e.g.:

{{ govukCharacterCount({
  "id": "with-translations",
  "name": "with-translations",
  "maxlength": 10,
  "label": {
    "text": "Full address"
  },
  "charactersUnderLimitHtml": {
    "one": "%{count} character to go",
    "other": "%{count} characters to go"
  },
  "charactersOverLimitHtml": {
    "one": "%{count} character too many",
    "other": "%{count} characters too many"
  },
  "charactersAtLimitHtml": "Zero characters to go"
}) }}

or:

new CharacterCount($element, {
  i18n: {
    charactersUnderLimit: {
      one: '%{count} character to go',
      other: '%{count} characters to go'
    },
    charactersOverLimit: {
      one: '%{count} character too many',
      other: '%{count} characters too many'
    },
    charactersAtLimit: 'Zero characters to go'
  }
}).init()

That'd make the data attributes e.g. data-i18n.characters-under-limit.one.

It does introduce some inconsistency with the charactersAtLimit option not taking nested config, but I think it's rational inconsistency as it makes sense when you think about it…

Thoughts?

romaricpascal · 2022-10-10T10:06:19Z

Oh dear, that was quite the oversight on my part, sorry for missing the other translation forms 😬 It will get repetitive indeed if we add one option for each plural form of each (it's already quite a bit) and using an object as you describe provides a much lighter alternative. Looks like it's a case of:

Nunjucks side: looping over the object and injecting one attribute per key. Wondering if it's not worth an pluralisedI18nAttribute macro to not burden the template with too much repetitive stuff.
JS side:
- Checking that the nested object in the config get flattened on multiple levels (and adding the feature if not)
- Update the I18n class to look for . separated plural forms rather than camelCased suffix
- Update the CharacterCount to match the new format, including
  - the default config
  - the associated tests
  - the constructor's JSDoc – with a PluralisedTranslation type for the translations

romaricpascal · 2022-10-10T10:16:10Z

The JS updates could be optional, as Nunjucks could be made to output attributes in our current format. But for consistency, best to have both be set up through objects/.-separated keys.

romaricpascal · 2022-10-10T10:17:32Z

@querkmachine I'd welcome a quick review of that plan as I may have missed a part in how the internationalisation works 😊

romaricpascal · 2022-10-10T15:12:54Z

@36degrees Second take on the matter, following the approach you suggested.

In terms of technical changes, the major thing beside the implementation of the feature itself are:

the introduction of a helper macro to render pluralised i18n attributes, as that was a good bit of boilerplate if we'd put it straight in the templates. I'm not attached to its naming, location or arguments (went with positional as it feels more performant than creating an object to just pass 2 options), so happy to discuss. Thinking it may be worth having a non-pluralised helper as well, and maybe move the {% if %} bits inside the helpers as well, but we can do that in a later PR.
the extraction of a callMacro helper to test that helper macro.

The documentation of the macro options hasn't been carried over yet from the first take, as I'd like to discuss the approach for it. From what I gathered, we currently have a system where we can document options as an object, then list the object's properties which will get extracted in the docs, like classes for the formGroup and countMessage options.

For internationalisation options, this will get very repetitive as it's the same set of options for all the options. I was thinking this is likely something that'll get described in the general documentation about configuration and internationalisation. If that's the case, it'll be lighter to just have a link pointing there rather than repeating 4 times the same list of 6 options.

@claireashworth how does this approach sound in terms of content organisation? Would it be a problem that, unlike the other options, we'd send users off the page to let them know that for charactersOver/UnderLimit and wordsOver/UnderLimit` they can pass 6 possible options, depending on their language needs?

@36degrees maybe there's another way I'm not aware to reduce duplication while documenting them on that same page, though?

querkmachine · 2022-10-10T15:17:12Z

@romaricpascal Only thing I was going to raise was to make sure the plural suffixes are the same format as the spec we're mimicking (zero, one, two, etc.) but it seems like you've already done that!

This'll let us use Objects to pass pluralised translations rather than one key per plural type

src/govuk/components/character-count/character-count.mjs

src/govuk/macros/i18n.njk

36degrees · 2022-10-12T08:54:39Z

src/govuk/macros/i18n.njk

@@ -0,0 +1,13 @@
+{#


Out of interest, is this following any sort of conventions for 'doc blocks' for Nunjucks macros, or is it something we've 'made up'?

I'm not aware of anything for Nunjucks doc blocks, nor found anything with a quick search, so went with following JSDocs conventions.

Just a thought – are we happy with this macro being part of the 'public API' for GOV.UK Frontend? It'd mean that if we made changes to it we'd need to treat them as breaking.

If not, we should try to be explicit about it being private. I don't believe Nunjucks has a concept of public/private for macros but we could for example rename it or put it in a folder called _private or something.

That's a good shout, I'll prefix it with _ and tag it @private in the doc which should make it clear that it's a function that we reserve to ourselves. I think I'd be keen to open it at the time we open the I18n API, as that'll be helpful for devs making their own macro to output attributes consistent with our API.

So... tough luck, Nunjucks is not happy with importing _ prefixed methods and complains with names starting with an underscore cannot be imported 😢 Tagging with @private in the doclet may be a bit light, but at least that we can do.

I'm a bit confused about what makes and doesn't make our public API though. Shouldn't anything that's not documented on either the design system site or frontend one be deemed private/"use as your own risk we may break it" anyway?

36degrees · 2022-10-12T08:55:39Z

src/govuk/macros/i18n.unit.test.mjs

+import { callMacro } from '../../../lib/jest-helpers'
+
+describe('i18n.njk', () => {
+  describe('govukPluralisedI18nAttributes', () => {


❤️ these tests

36degrees · 2022-10-12T08:56:21Z

src/govuk/macros/i18n.unit.test.mjs

+      // Note the starting space so we ensure it doesn't stick to possible other previous attributes
+      expect(attributes).toEqual(' data-i18n.translation-key.other="You have %{count} characters remaining."')
+    })
+    it('renders multiple plural types', () => {


This is really picky, and definitely a personal preference thing, but I find it considerably easier to read blocks of tests if there's an empty new line between each test.

No, that's a fair point,I think. I tend to write things quite compact.

On a side note, that could be something we lint for with ESLint's padding-line-between-statement, to enforce a blank line after multiline-expression (and I'd say multiline anything, to be honest).

Yep, I've had a similar conversation with Colin about this on Slack.

Aware ideally this sort of thing wouldn't be coming up in code review – lots of stuff we can improve / automate here.

src/govuk/common.unit.test.mjs

src/govuk/i18n.mjs

romaricpascal · 2022-10-13T08:57:24Z

☝🏻 Removed two async in tests that weren't awaiting anything.

colinrotherham · 2022-10-13T09:04:50Z

lib/jest-helpers.js

+ *
+ * @param {string} macroName - The name of the macro
+ * @param {string} macroPath - The path to the file containing the macro *from the root of the project*
+ * @param {Array} options - The parameters that will be passed to the macro. They'll be `JSON.stringify`ed and joined with a `,`


Just a JSDoc suggestion, can we start explaining what it's an array of?

Strings, numbers, objects?

Slightly better?

Suggested change

* @param {Array} options - The parameters that will be passed to the macro. They'll be `JSON.stringify`ed and joined with a `,`

* @param {object[]} options - The parameters that will be passed to the macro. They'll be `JSON.stringify`ed and joined with a `,`

A lot better?

Suggested change

* @param {Array} options - The parameters that will be passed to the macro. They'll be `JSON.stringify`ed and joined with a `,`

* @param {{ key1: string; key2: number }[]} options - The parameters that will be passed to the macro. They'll be `JSON.stringify`ed and joined with a `,`

Not exactly sure, but flat structure?
Unless there are some unknown nested objects, can show a flat key/value object too

Suggested change

* @param {Array} options - The parameters that will be passed to the macro. They'll be `JSON.stringify`ed and joined with a `,`

* @param {Object<string, string | boolean | number>[]} options - The parameters that will be passed to the macro. They'll be `JSON.stringify`ed and joined with a `,`

Really not sure, but always string keys
Might have to check this with the TypeScript compiler (might want unknown or any) but in JSDoc you can say we've always got string keys but any value

Suggested change

* @param {Array} options - The parameters that will be passed to the macro. They'll be `JSON.stringify`ed and joined with a `,`

* @param {Object<string, *>[]} options - The parameters that will be passed to the macro. They'll be `JSON.stringify`ed and joined with a `,`

That one is really an Array<any>. Its content will depend on the shape of the macro params that's under test.

const macroParams = options.map(param => JSON.stringify(param, null, 2)).join(',')

Makes me think the naming is not necessarily ideal, as options is usually an object, so I'll swap for params which should clarify :)

Sorry, there's a [] after my Object<> examples, "array of objects" shorthand

Yeah it does looks like it's usually (always?) an object[] or undefined

It could be an array of anything, not just object as a macro may accept number, booleans, arrays, objects as any of its parameters.

colinrotherham · 2022-10-13T09:10:32Z

src/govuk/components/character-count/character-count.mjs

@@ -5,6 +5,42 @@ import '../../vendor/polyfills/Element/prototype/classList.mjs'
 import { closestAttributeValue, extractConfigByNamespace, mergeConfigs, normaliseDataset } from '../../common.mjs'
 import { I18n } from '../../i18n.mjs'

+/**
+ * @typedef {object} CharacterCountTranslations


I've been keeping extra JSDoc comments at the bottom:

https://github.com/alphagov/govuk-frontend/blob/eslint-jsdoc/lib/file-helper.js#L151
https://github.com/alphagov/govuk-frontend/blob/eslint-jsdoc/src/govuk/common.mjs#L218

Do we want to decide if "at the top" is preferred?

colinrotherham · 2022-10-13T09:15:25Z

src/govuk/components/character-count/character-count.mjs

+/**
+ * @type {CharacterCountTranslations}
+ */
+var DEFAULT_TRANSLATIONS = {


No comment on the word "translations", but similar to HEROKU_APP, GITHUB_TOKEN, PERCY_TOKEN, KEY_SPACE etc I'd put the type/modifier after the thing (if that makes sense 😆)

Suggested change

var DEFAULT_TRANSLATIONS = {

var TRANSLATIONS_DEFAULT = {

colinrotherham · 2022-10-13T09:32:52Z

src/govuk/components/character-count/template.njk

 {% from "../textarea/macro.njk" import govukTextarea %}
 {% from "../hint/macro.njk" import govukHint %}

 <div class="govuk-character-count" data-module="govuk-character-count"
 {%- if params.maxlength %} data-maxlength="{{ params.maxlength }}"{% endif %}
 {%- if params.threshold %} data-threshold="{{ params.threshold }}"{% endif %}
-{%- if params.maxwords %} data-maxwords="{{ params.maxwords }}"{% endif %}>
+{%- if params.maxwords %} data-maxwords="{{ params.maxwords }}"{% endif %}
+{%- if params.charactersUnderLimitHtml %}{{govukPluralisedI18nAttributes('characters-under-limit', params.charactersUnderLimitHtml)}}{% endif %}


Super quick not-even-tried-it example, but how about a function that does all i18n attributes?

{% set i18n = params.i18n %}

{%- if i18n %}{{ govukI18nAttributes([ ['characters-under-limit', i18n.charactersUnderLimitHtml], ['characters-at-limit', i18n.charactersAtLimitHtml], ['characters-over-limit', i18n.charactersOverLimitHtml], ['words-under-limit', i18n.wordsUnderLimitHtml], ['words-at-limit', i18n.wordsAtLimitHtml], ['words-over-limit', i18n.wordsOverLimitHtml] ])}}{% endif %}

The idea is appealing, and we don't really need to introduce a separate i18n namespace for them, we can just pick the values from params directly as we do have to read them manually (unless someone knows of some black magic to go kebab-case to cameCase or the other way around with Nunjucks without adding a filter).

There's a little trickiness to it as translations can be strings or object, depending of if they're pluralised or not. Using the current helper, the template makes it clear which attribute expects which, so we'd need to devise an API that lets us do the same, maybe:

{{ govukI18nAttributes({ 'characters-under-limit': {plurals: {params.charactersUnderLimitHtml}}, 'characters-over-limit': {plurals: {params.charactersOverLimitHtml}}, 'characters-at-limit': {string: params.charactersAtLimitHtml}, 'words-under-limit': {plurals: {params.wordsUnderLimitHtml}}, 'words-over-limit': {plurals: {params.wordsOverLimitHtml}}, 'words-at-limit': {string: params.wordsAtLimitHtml} })}}

That said, I think there's ground for a codebase wise look at some abstraction around data attributes, like a govukDataAttributes helper on a similar model, as setting them is always the same thing:

check if the value is truthy (or not undefined in a few cases)

set data-{{name}}={{value}} (sometimes escaped)

Added an issue to track: #2917 😄

@romaricpascal Yeah, look less at the "how" and more of a nice neat way to generate attributes

We already have this syntax elsewhere for .attributes:

{%- for attribute, value in message.attributes %} {{attribute}}="{{value}}"{% endfor %}

Then other "ad-hoc" data attributes in places:

{%- if params.disableAutoFocus !== undefined %} data-disable-auto-focus="{{ params.disableAutoFocus }}"{% endif %}

If each template had a map (or object) of all its available attributes, defaults, parameter overrides etc, you could write both regular and data attributes into the template in a nice unit-testable way

This certainly isn't a blocker, just an opportunity to makes the template look neater

Co-authored-by: Oliver Byford <[email protected]>

This part of the execution flow is not currently tested as `Intl.PluralRules` exists in the test environment, so we never follow the 'else' part of the control flow. Splitting it out allows us to unit test it and ensure that it works as expected. Add error handling for unsupported locales.

This allows us to test the entire fallback logic together, passing a locale and asserting that we have the same behaviour as Intl.PluralRules, rather than testing the individual functions in i18n.PluralRules.

Improve test coverage of the I18n class

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2895 October 3, 2022 16:00 Inactive

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2895 October 3, 2022 16:10 Inactive

romaricpascal requested a review from a team October 3, 2022 16:30

romaricpascal marked this pull request as ready for review October 3, 2022 16:30

36degrees reviewed Oct 7, 2022

View reviewed changes

romaricpascal force-pushed the character-count-i18n branch from a47c9d8 to 82bdf28 Compare October 10, 2022 08:56

romaricpascal mentioned this pull request Oct 10, 2022

Allow CharacterCount component to receive i18n config via JS #2887

Merged

romaricpascal force-pushed the character-count-i18n-macro branch from 1dc02d6 to 18f4b34 Compare October 10, 2022 14:41

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2895 October 10, 2022 14:41 Inactive

romaricpascal force-pushed the character-count-i18n-macro branch from 18f4b34 to b328723 Compare October 10, 2022 14:55

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2895 October 10, 2022 14:55 Inactive

romaricpascal requested a review from 36degrees October 10, 2022 15:12

romaricpascal force-pushed the character-count-i18n branch from 82bdf28 to 3a42be1 Compare October 11, 2022 14:28

romaricpascal force-pushed the character-count-i18n-macro branch from b328723 to a66be98 Compare October 11, 2022 14:43

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2895 October 11, 2022 14:43 Inactive

36degrees mentioned this pull request Oct 11, 2022

Work out how to document different plural rules in the Nunjucks option table for the Character Count alphagov/govuk-design-system#2378

Closed

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2895 October 11, 2022 15:09 Inactive

romaricpascal force-pushed the character-count-i18n branch from 727dbdb to a5ed606 Compare October 11, 2022 15:13

romaricpascal added 2 commits October 11, 2022 16:13

Update I18n to use '.' separated plural suffix

7a4d712

This'll let us use Objects to pass pluralised translations rather than one key per plural type

Test config key flattening is on multiple levels

d7993a0

romaricpascal force-pushed the character-count-i18n-macro branch from e2a60b7 to 0034021 Compare October 11, 2022 15:14

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2895 October 11, 2022 15:14 Inactive

Base automatically changed from character-count-i18n to main October 11, 2022 15:30

36degrees reviewed Oct 12, 2022

View reviewed changes

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2895 October 12, 2022 09:42 Inactive

colinrotherham reviewed Oct 13, 2022

View reviewed changes

romaricpascal force-pushed the character-count-i18n-macro branch from 0b1089a to 4832d14 Compare October 13, 2022 12:12

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2895 October 13, 2022 12:12 Inactive

romaricpascal mentioned this pull request Oct 13, 2022

Introduce Nunjucks macros abstracting data attribute rendering #2917

Closed

2 tasks

romaricpascal and others added 5 commits October 13, 2022 16:11

Make CharacterCount use the dot separated plural forms

e3a889e

Add helper macro for outputing i18n pluralisations

d88f1fa

Add options for passing translations to character count macro

75d6527

Fix typing of CharacterCount translations

5a01101

Fix typos

cd41407

Co-authored-by: Oliver Byford <[email protected]>

romaricpascal force-pushed the character-count-i18n-macro branch from 4832d14 to cd41407 Compare October 13, 2022 15:14

govuk-design-system-ci had a problem deploying to govuk-frontend-pr-2895 October 13, 2022 15:14 Failure

Mark govukPluralisedI18nAttributes as private

7fac761

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2895 October 13, 2022 15:28 Inactive

colinrotherham mentioned this pull request Oct 13, 2022

Character Count component throws error with missing plural text #2918

Closed

36degrees added 7 commits October 14, 2022 16:59

Split out fallback logic for selecting plural rule

c16fa49

This allows us to test the entire fallback logic together, passing a locale and asserting that we have the same behaviour as Intl.PluralRules, rather than testing the individual functions in i18n.PluralRules.

Simplify plural rule tests by using it.each

ee5882d

Organise unit tests by called function

db8a51f

Simplify getPluralSuffix logic

1376462

Move pluralRules and pluralRulesMap to class

cd15d29

Merge pull request #2922 from alphagov/i18n-unit-test-coverage

bfccd7e

Improve test coverage of the I18n class

govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2895 October 14, 2022 16:08 Inactive

colinrotherham approved these changes Oct 17, 2022

View reviewed changes

romaricpascal merged commit 023c3bf into main Oct 17, 2022

romaricpascal deleted the character-count-i18n-macro branch October 17, 2022 09:01

romaricpascal mentioned this pull request Nov 10, 2022

Release v4.4.0 #3000

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add macro options to configure CharacterCount translations #2895

Add macro options to configure CharacterCount translations #2895

romaricpascal commented Oct 3, 2022 •

edited

Loading

36degrees left a comment •

edited

Loading

romaricpascal commented Oct 10, 2022 •

edited

Loading

romaricpascal commented Oct 10, 2022

romaricpascal commented Oct 10, 2022

romaricpascal commented Oct 10, 2022

querkmachine commented Oct 10, 2022

36degrees Oct 12, 2022

romaricpascal Oct 12, 2022

36degrees Oct 13, 2022

romaricpascal Oct 13, 2022

romaricpascal Oct 13, 2022

36degrees Oct 12, 2022

36degrees Oct 12, 2022

romaricpascal Oct 12, 2022

36degrees Oct 12, 2022

romaricpascal commented Oct 13, 2022

colinrotherham Oct 13, 2022

romaricpascal Oct 13, 2022

colinrotherham Oct 13, 2022

romaricpascal Oct 13, 2022

colinrotherham Oct 13, 2022

colinrotherham Oct 13, 2022

colinrotherham Oct 13, 2022

romaricpascal Oct 13, 2022

romaricpascal Oct 13, 2022

colinrotherham Oct 13, 2022

	* @param {Array} options - The parameters that will be passed to the macro. They'll be `JSON.stringify`ed and joined with a `,`
	* @param {object[]} options - The parameters that will be passed to the macro. They'll be `JSON.stringify`ed and joined with a `,`

	* @param {Array} options - The parameters that will be passed to the macro. They'll be `JSON.stringify`ed and joined with a `,`
	* @param {{ key1: string; key2: number }[]} options - The parameters that will be passed to the macro. They'll be `JSON.stringify`ed and joined with a `,`

	* @param {Array} options - The parameters that will be passed to the macro. They'll be `JSON.stringify`ed and joined with a `,`
	* @param {Object<string, string \| boolean \| number>[]} options - The parameters that will be passed to the macro. They'll be `JSON.stringify`ed and joined with a `,`

	* @param {Array} options - The parameters that will be passed to the macro. They'll be `JSON.stringify`ed and joined with a `,`
	* @param {Object<string, *>[]} options - The parameters that will be passed to the macro. They'll be `JSON.stringify`ed and joined with a `,`

Add macro options to configure CharacterCount translations #2895

Add macro options to configure CharacterCount translations #2895

Conversation

romaricpascal commented Oct 3, 2022 • edited Loading

36degrees left a comment • edited Loading

Choose a reason for hiding this comment

romaricpascal commented Oct 10, 2022 • edited Loading

romaricpascal commented Oct 10, 2022

romaricpascal commented Oct 10, 2022

romaricpascal commented Oct 10, 2022

querkmachine commented Oct 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romaricpascal commented Oct 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romaricpascal commented Oct 3, 2022 •

edited

Loading

36degrees left a comment •

edited

Loading

romaricpascal commented Oct 10, 2022 •

edited

Loading