govuk-frontend icon indicating copy to clipboard operation
govuk-frontend copied to clipboard

Add ability to pass translation strings into character count component via data-attributes

Open vanitabarrett opened this issue 1 year ago • 2 comments

What

Add the ability to pass translation strings into the character count component, using the Nunjucks macro. The logic to lookup translation strings in data attributes and get them in a JS object format should already have been covered by https://github.com/alphagov/govuk-frontend/issues/2699 , so this should just be a case of adding the Nunjucks macro options to the component and any changes to the component JS to switch out the hardcoded strings.

Why

So user can translate the hardcoded strings in our character count component. Passing translations via the Nunjucks macro enables them to do this without having to understand where different strings are (e.g: Nunjucks vs JavaScript) as it’s the same interface for both.

Who needs to work on this

Developers

Who needs to review this

Developers

Done when

  • [ ] The character count component has new Nunjucks macro options for passing translation strings (following the format decided on in https://github.com/alphagov/govuk-frontend/issues/2780 )
  • [ ] The character count component uses the internationalisation logic to swap out hardcoded strings
  • [ ] The character count component still works if no/only some translations are passed (falls back to defaults)
  • [ ] The character count component has tests which cover the new ability to translate
  • [ ] Added or created a full page example which shows full translation of the character count component

vanitabarrett avatar Jul 15 '22 09:07 vanitabarrett

As the component is relying on pluralisation, should it also receive a locale through data attributes to decide on which plural form to use? Was there any discussion around the shape for that attribute? Translations are passed as data-i18n.translationKey so data-i18n.locale would cause a collision if a components translation key was locale for some reason (though I guess we can always reserve some keys within that i18n namespace?).

romaricpascal avatar Sep 13 '22 14:09 romaricpascal

I think mixing them in together is probably fine. We already have count, although arguably that is both 'config' and a replacement.

Probably need to check with @querkmachine to see how intentional it is, but a lot of the design decisions we've made so far have mimicked i18next – including the fact that replacements can be mixed in with options (see https://www.i18next.com/translation-function/essentials#overview-options), although for us it's the only way to pass them at the minute.

(As touched on in https://github.com/alphagov/govuk-frontend/issues/2804#issuecomment-1245335164, an alternative approach would be to use the lang attribute instead, but that'd mean there'd be no way to set the locale in JavaScript. Maybe that's OK?)

36degrees avatar Sep 13 '22 15:09 36degrees

Ended up having a separate 'i18nLocale' option that falls back to 'lang' on the JavaScript side.

Was there ever a discussion on passing the locale to the Nunjucks template or are we just expecting users to set attributes: {lang: ...} in the macro options? The discussion on the API seems to only deal with translation strings.

romaricpascal avatar Sep 29 '22 15:09 romaricpascal

Was there ever a discussion on passing the locale to the Nunjucks template or are we just expecting users to set attributes: {lang: ...} in the macro options? The discussion on the API seems to only deal with translation strings.

I would expect in the vast majority of cases, the correct implementation would have the character count inside a parent element that has the lang attribute – potentially on the body or the main element. Although I think we should support it, I can think of few cases where I'd actually expect there to be a lang element on the character count component itself.

I think adding a top-level lang Nunjucks option would encourage users to specify a lang attribute even when it's not needed, which might potentially cause problems if things get out of sync.

On that basis, and given it can be set using the attributes object if it is needed, I don't think we should add it to the top level.

36degrees avatar Sep 29 '22 16:09 36degrees

Good point of avoiding to encourage people of always setting a lang through a top level lang option in the macro options. That said, we're in a pinch with using attributes as it gets passed to the textarea component.

I couldn't find any occurences of something like wrapperAttributes or rootAttributes options in other components, so I guess that's the first time we're having to set attributes on both something inside the component and the root element of the component. From what I gathered all the other components use attributes to set attributes on their root element.

If we wanted to be super clean in naming, which I'd be keen on, I'd like to move to having attributes for the root element and textareaAttributes for the textarea and get the component in line with the rest of the others. However, this is a breaking change.

We could start with:

  1. adding a wrapperAttributes option to not have a lang or locale option at the top level encouraging people to set it
  2. introduce a textareaAttributes option and use its presence to ignore wrapperAttributes, use attributes on the root and textareaAttributes on the textarea
  3. if it's possible warn users using attributes without textareaAttributes that we're looking to update where those attributes get applied and they should move to the new option.

I guess 2 and 3 are a little like the updates to the character count attributes and could be done in 5.x for the final swap happening in 6.x if we're happy with the wrapperAttributes name.

romaricpascal avatar Sep 29 '22 16:09 romaricpascal

@romaricpascal I agree that the inconsistent use of the attributes parameter in the character count is something we should fix, but strictly speaking it's not a prerequisite for the internationalisation work, as a developer could just wrap the Nunjucks macro in a div with the lang attribute if they need to translate this single component separate to the rest of the page—which is not a hotly anticipated use case.

I'd suggest opening a new issue to resolve the attributes inconsistency, but not worrying about it for this piece of work.

querkmachine avatar Oct 03 '22 12:10 querkmachine

If we're happy with letting the consuming code wrap, that's fine by me as well 😄

Created https://github.com/alphagov/govuk-frontend/issues/2893 to track the attributes inconsitency.

romaricpascal avatar Oct 03 '22 13:10 romaricpascal

Reopening as the macro PR is still open

romaricpascal avatar Oct 12 '22 09:10 romaricpascal

Re-opened as I think we still need a full page translated example for it. There's a non-hidden test example for it. If that's enough, we can close this issue, I think.

romaricpascal avatar Oct 17 '22 13:10 romaricpascal