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

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

Closed
4 of 5 tasks
Tracked by #1708
vanitabarrett opened this issue Jul 15, 2022 · 9 comments · Fixed by #2887, #2895 or #2934
Closed
4 of 5 tasks
Tracked by #1708

Comments

@vanitabarrett
Copy link
Contributor

vanitabarrett commented Jul 15, 2022

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 #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 Decide JavaScript and Nunjucks API for passing translation strings #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 vanitabarrett changed the title Add ability to translate hardcoded strings in character count component JavaScript Add ability to pass translation strings into character count component via data-attributes Aug 22, 2022
@domoscargin domoscargin self-assigned this Sep 6, 2022
@domoscargin domoscargin removed their assignment Sep 8, 2022
@36degrees 36degrees added this to the [NEXT] milestone Sep 12, 2022
@romaricpascal
Copy link
Member

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?).

@36degrees
Copy link
Contributor

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 #2804 (comment), 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?)

@romaricpascal
Copy link
Member

romaricpascal commented Sep 29, 2022

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.

@36degrees
Copy link
Contributor

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.

@romaricpascal
Copy link
Member

romaricpascal commented Sep 29, 2022

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.

@querkmachine
Copy link
Member

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

@romaricpascal
Copy link
Member

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

Created #2893 to track the attributes inconsitency.

@romaricpascal
Copy link
Member

Reopening as the macro PR is still open

@romaricpascal
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment