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

Namespace should not be translated #1436

Merged
merged 1 commit into from
May 27, 2020

Conversation

AlanGreene
Copy link
Member

Changes

#453

The word 'Namespace' should not be translated as it's a
Kubernetes kind, and should be treated similar to the others,
e.g. PipelineRun, Task, ServiceAccount

This also resolves a react-intl warning about missing messages
for consumers who are not exposing the namespace column in our
tables, e.g.:

[React Intl] Missing message: "dashboard.tableHeader.namespace"
for locale: "fr", using default message as fallback.

They shouldn't have to provide a value for a feature they're
not using so this addresses both issues in one. It also gives us
consistency as we were already not translating it in some places.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

See the contribution guide
for more details.

The word 'Namespace' should not be translated as it's a
Kubernetes kind, and should be treated similar to the others,
e.g. PipelineRun, Task, ServiceAccount

This also resolves a react-intl warning about missing messages
for consumers who are not exposing the namespace column in our
tables, e.g.:

```
[React Intl] Missing message: "dashboard.tableHeader.namespace"
for locale: "fr", using default message as fallback.
```

They shouldn't have to provide a value for a feature they're
not using so this addresses both issues in one.
@tekton-robot tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 26, 2020
@AlanGreene AlanGreene requested review from a-roberts and eddycharly and removed request for akihikokuroda and ncskier May 26, 2020 12:02
Copy link
Member

@a-roberts a-roberts left a comment

Choose a reason for hiding this comment

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

Missing one I think
https://github.com/tektoncd/dashboard/blob/797e0ebca0669f536989ca026ff1d3dc984e7d21/packages/components/src/components/Trigger/Trigger.js

   const namespaceText = intl.formatMessage({
                id: 'dashboard.triggerDetails.interceptorNamespace',
                defaultMessage: 'Namespace:'
              });

(so, my search is https://github.com/tektoncd/dashboard/search?q=defaultMessage%3A+%27Namespace%27&unscoped_q=defaultMessage%3A+%27Namespace%27)

Also there are some instances where we do this (and so surely namespace would be translated there when it's really a resource as you say, so something to think about or follow-up on?)

23 | import NamespacesDropdown from '../../../containers/NamespacesDropdown';
-- | --
24 |  
25 | const UniversalFields = props => {
26 | const {
27 | handleChangeNamespace,
… |  
72 | id: 'dashboard.universalFields.namespaceInvalid',
73 | defaultMessage: 'Namespace required.'

@AlanGreene
Copy link
Member Author

AlanGreene commented May 26, 2020

Yeah great question, i18n is a fun and sometimes tricky business 😆

Where 'Namespace' appears as part of another string we'll still need to include it in messages.json, that includes the 'Namespace:' label for the Trigger interceptors, as the surrounding string may change, e.g. in some languages : may be substituted for another character, moved to the start of the string, or removed altogether.

We would provide a list of terms that should not be translated when used in certain contexts. Normally this would include things like branding tokens like 'Tekton', 'Dashboard', etc., but we would include other things like the list of Kubernetes kinds too, or specific terms / acronyms like 'YAML'.

So the upshot is we need to continue handling these strings as we do today, and document exceptions for translators who would be working from the messages_en.json (normally via some tooling) to produce the translated versions.

@AlanGreene
Copy link
Member Author

@a-roberts this should be good to go

@a-roberts a-roberts self-requested a review May 27, 2020 09:41
Copy link
Member

@a-roberts a-roberts left a comment

Choose a reason for hiding this comment

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

/lgtm
/meow boxes

@tekton-robot
Copy link
Contributor

@a-roberts: cat image

In response to this:

/lgtm
/meow boxes

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 27, 2020
@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: a-roberts

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 27, 2020
@tekton-robot tekton-robot merged commit da086e1 into tektoncd:master May 27, 2020
@AlanGreene AlanGreene deleted the i18n_namespace branch May 27, 2020 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants