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

Translate UI Schema elements #1966

Merged
merged 1 commit into from
Jul 7, 2022

Conversation

sdirix
Copy link
Member

@sdirix sdirix commented Jun 28, 2022

'Group', 'Category' and 'Label' UI Schema elements are now translatable via bindings.

'mapStateToLayoutProps' is enhanced to check for 'label' or 'i18n' attributes within the
UI Schema element. If that's the case the translator will be invoked and the label handed
over which is then used by the group renderers.

The categorization renderers access the new utility 'deriveLabelForUISchemaElement' to
translate the labels of their categories. This is done within the respective renderer
code.

For labels a new mapStateToLabelProps mapper is added which provides access to a 'text'
property. Corresponding bindings are added to all renderer sets and used by the
respective renderers.

Also improves some typings in core and streamlines prop handling in the React bindings.

'Group', 'Category' and 'Label' UI Schema elements are now translatable via bindings.

'mapStateToLayoutProps' is enhanced to check for 'label' or 'i18n' attributes within the
UI Schema element. If that's the case the translator will be invoked and the label handed
over which is then used by the group renderers.

The categorization renderers access the new utility 'deriveLabelForUISchemaElement' to
translate the labels of their categories. This is done within the respective renderer
code.

For labels a new `mapStateToLabelProps` mapper is added which provides access to a 'text'
property. Corresponding bindings are added to all renderer sets and used by the
respective renderers.

Also improves some typings in core and streamlines prop handling in the React bindings.
@sdirix sdirix requested a review from LukasBoll June 28, 2022 08:04
@sdirix sdirix linked an issue Jun 28, 2022 that may be closed by this pull request
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 84.259% when pulling 126d902 on sdirix:translate-uischema-elements into d2bf053 on eclipsesource:master.

Copy link
Contributor

@lucas-koehler lucas-koehler left a comment

Choose a reason for hiding this comment

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

Code changes LGTM. I just have one inline question. I did not test the changes.

const t = getTranslator()(state);
const i18nKeyPrefix = getI18nKeyPrefixBySchema(undefined, uischema);
const i18nKey = i18nKeyPrefix ? `${i18nKeyPrefix}.text` : text ?? '';
const i18nText = t(i18nKey, text, { uischema });
Copy link
Contributor

Choose a reason for hiding this comment

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

In case there is no i18nKeyPrefix determined in line 1097, it seems that text is effectively used as the first and second parameter here. Thus, the text would be used as an i18n key (assigned in line 1098), right? Is that on purpose. It seems to me that this could lead to unexpected behavior: E.g. There is a plain text that should be shown. By chance it matches any i18n key => The translation of this key is shown.
Or am I missing something?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes that's on purpose. We do the same for error messages.

I don't know if there is a better solution, do you have something in mind? The idea is that this allows users to specify their i18n keys directly within the label if they want to.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay that makes sense. Unfortunately, I cannot think of a better solution that allows providing the i18n key in a similarly simple fashion.
I guess the collision risk is reasonably low as long as users use typical formats for such keys. We can leave it like this, I just wanted to clarify :)

Copy link
Contributor

@lucas-koehler lucas-koehler Jul 5, 2022

Choose a reason for hiding this comment

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

There's one approach I can think of: If the label's value should be resolved to a translation, it could be marked as a variable. E.g. like one of these:

  • %mykey%
  • ${mykey}

This allows to specify the key inline in the label prop and still makes it explicit that this should be resolved.

Copy link
Member Author

@sdirix sdirix Jul 5, 2022

Choose a reason for hiding this comment

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

In theory these could also be collisions ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

I know but it makes it less likely and it is explicit that a resolvement is expected.
But I'm also fine with leaving it as is as the collision risk should be very low for reasonable users ;)

Copy link
Contributor

@lucas-koehler lucas-koehler left a comment

Choose a reason for hiding this comment

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

I tested it for vanilla, material and angular material via the examples.
The first two worked great. However, for angular material only the i18n keys itself were shown (both when specifying via i18n or in the label property).

Does it work for you?

@LukasBoll
Copy link
Contributor

I tested it for vanilla, material and angular material via the examples. The first two worked great. However, for angular material only the i18n keys itself were shown (both when specifying via i18n or in the label property).

Does it work for you?

Hi,
I also tested it with the example app. For me it worked for all renders, also the angular material renderer.

Copy link
Contributor

@lucas-koehler lucas-koehler left a comment

Choose a reason for hiding this comment

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

Turned out I didn't properly propagate my test i18n config to the angular material example app. Now it worked :)

@sdirix sdirix merged commit 6a6af7e into eclipsesource:master Jul 7, 2022
@sdirix sdirix deleted the translate-uischema-elements branch July 7, 2022 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Translate labeled UI Schema elements
4 participants