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 initial i18n documentation #222

Merged
merged 9 commits into from
Oct 13, 2022
Merged

Conversation

TheZoker
Copy link
Contributor

@TheZoker TheZoker commented Apr 4, 2022

Adds initial i18n documentation

Closes #207

Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

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

I added some suggestions. Please add them as you see fit and make sure the document is in a good state overall.

content/docs/i18n.mdx Outdated Show resolved Hide resolved
content/docs/i18n.mdx Outdated Show resolved Hide resolved
Comment on lines 123 to 126
The paths in the above example would be:
- `firstName.label` & `firstName.description`
- `address.street.label` & `address.street.description`
- `comments.message.label` & `comments.message.description` (the path for arrays will not contain indices)
Copy link
Member

Choose a reason for hiding this comment

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

Can you check whether address.label, address.description and comments.label, comments.description is also called?

content/docs/i18n.mdx Outdated Show resolved Hide resolved
content/docs/i18n.mdx Outdated Show resolved Hide resolved
content/docs/i18n.mdx Outdated Show resolved Hide resolved
content/docs/i18n.mdx Outdated Show resolved Hide resolved
content/docs/i18n.mdx Outdated Show resolved Hide resolved
content/docs/i18n.mdx Outdated Show resolved Hide resolved
content/docs/i18n.mdx Outdated Show resolved Hide resolved
Co-authored-by: Stefan Dirix <sdirix@eclipsesource.com>
@TheZoker TheZoker requested a review from sdirix April 12, 2022 10:49
@sdirix sdirix requested a review from LukasBoll April 12, 2022 10:52
@sdirix
Copy link
Member

sdirix commented Apr 12, 2022

@LukasBoll

With this documentation, please try to implement translation support in the JSON Forms React seed (first update to the latest JSON Forms).

Please review and identify potential problems with the current documentation. Please test exhaustively and question all information. Try all kind of translations (usual, errors, enums) and using the translation support in a custom renderer.

@LukasBoll
Copy link
Contributor

Documentation looks good to me!
Currently Enum options only change, when the translation function changes, but not if only the locale changes. Maybe we should address the in the future.

@TheZoker TheZoker marked this pull request as ready for review May 10, 2022 23:27
@sdirix
Copy link
Member

sdirix commented Jul 12, 2022

  • Document new UI schema element translation, see here
  • Document handed over parameters (look through code base), e.g. schema, path, uischema etc.

Signed-off-by: Lukas Boll lukas-bool@web.de
@sdirix sdirix mentioned this pull request Sep 21, 2022
16 tasks
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.

Looks already pretty good to me.
Please note that the i18n prop is now directly defined in control elements (see inline comment).
Furthermore, I provided some grammar/wording/typo fixes I noticed ;)

(key: string, defaultMessage: string | undefined, context: any) => string | undefined)
```

and there for has the following parameters:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
and there for has the following parameters:
with the following parameters:


## `locale`

Allows to specify the current locale of the application.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Allows to specify the current locale of the application.
Specifies the current locale of the application.


## `translate`

Allows to provide a translation function, which handles the actual translation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Allows to provide a translation function, which handles the actual translation.
Provides a translation function handling the actual translation.

/>
```

The i18n prop consist of three components: `locale`, `translate` and `translateError`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The i18n prop consist of three components: `locale`, `translate` and `translateError`.
The `i18n` prop consists of three components: `locale`, `translate` and `translateError`.


### `key`

The key is used to identify the string, that needs to be translated.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The key is used to identify the string, that needs to be translated.
The key is used to identify the string that needs to be translated.

}
```

Therefore the translation will be invoked with `myCustomName.label` & `myCustomName.description`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Therefore the translation will be invoked with `myCustomName.label` & `myCustomName.description`.
Therefore, the translation will be invoked with `myCustomName.label` & `myCustomName.description`.

}
```

The paths in the above example would be:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The paths in the above example would be:
The paths in the above example are:


:::note

If the `defaultMessage` is `undefined` you should also return `undefined` if you don't have a translation for the given key.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
If the `defaultMessage` is `undefined` you should also return `undefined` if you don't have a translation for the given key.
If the `defaultMessage` is `undefined`, you should also return `undefined` if there is no translation for the given key.


<ValuesTable/>

Schema translation provide all properties, while UI schema translations only provide the `uischema`-property.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Schema translation provide all properties, while UI schema translations only provide the `uischema`-property.
Schema translations provide all properties, while UI schema translations only provide the `uischema` property.


If a i18n-key is provided in a `group` or `category` element, `<i18n>.label` will be used as key.
If no i18n key is provided, the value of the `label`-property is used as key.
In case nether a i18n-key nor a label is provided, `<property-path>.label` will be used as default key.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
In case nether a i18n-key nor a label is provided, `<property-path>.label` will be used as default key.
In case neither a i18n-key nor a label is provided, `<property-path>.label` will be used as default key.

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.

LGTM now, thanks for the updates!

@lucas-koehler lucas-koehler removed the request for review from sdirix October 13, 2022 07:45
@lucas-koehler lucas-koehler merged commit c5fe90f into eclipsesource:master Oct 13, 2022
@sdirix sdirix mentioned this pull request Jan 9, 2024
14 tasks
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.

Document new translation (i18n) support
4 participants