Add i18n to topology package #7315

jeff-phillips-18 · 2020-11-24T15:49:49Z

Fixes: https://issues.redhat.com/browse/ODC-5130

jeff-phillips-18 · 2020-11-24T18:50:55Z

/retest

andrewballantyne · 2020-11-26T19:20:47Z

frontend/packages/topology/src/utils/moveNodeToGroup.tsx

+        <strong>{node.getLabel()}</strong>
+        {targetGroup
+          ? t('topology~ from {{sourceGroup}} to  {{targetGroup}}?', {
+              sourceGroup: sourceGroup.getLabel(),
+              targetGroup: targetGroup.getLabel(),
+            })
+          : t('topology~ from {{sourceGroup}}?', { sourceGroup: sourceGroup.getLabel() })}


🤔 There is a pattern that can be used here...

Quick search through the codebase got me:

"teams-help": "Optionally list organizations. If specified, only GitHub users that are members of at least one of the listed organizations will be allowed to log in. Cannot be used in combination with <1>teams</1>.",

And:

<Trans i18nKey="teams-help" ns="github-idp-form"> Optionally list organizations. If specified, only GitHub users that are members of at least one of the listed organizations will be allowed to log in. Cannot be used in combination with <strong>teams</strong>. </Trans>

Ref: github-idp-form.tsx

cc @christianvogt might know more -- I'm reverse engineering here.

andrewballantyne · 2020-11-26T19:22:41Z

frontend/packages/topology/src/actions/groupActions.ts

@@ -29,15 +30,15 @@ const cleanUpWorkloadNode = async (workload: OdcNodeModel): Promise<K8sResourceK
  return cleanUpWorkload(workload.resource, workload.data?.isKnativeResource ?? false);
 };

-const deleteGroup = (application: TopologyApplicationObject) => {
+const deleteGroup = (application: TopologyApplicationObject, t: TFunction) => {


Interesting, you put t at the start of other methods 🤔

I have no opinion one way or another, just thought it was noteworthy.

andrewballantyne · 2020-11-26T19:30:56Z

frontend/packages/topology/src/components/modals/MoveConnectionModal.tsx

+          {t('topology~Connect')}
          <strong>{` ${edge.getSource().getLabel()} `}</strong>
-          to
+          {t('topology~to')}


This should be one translation otherwise it won't come out properly in other languages.

andrewballantyne · 2020-11-26T19:43:56Z

frontend/packages/topology/src/operators/TopologyServiceBindingRequestPanel.tsx

+
+const ConnectedTopologyServiceBindingRequestPanel: React.FC<PropsFromState &
+  PropsFromDispatch &
+  TopologyServiceBindingRequestPanelProps> = ({ edge, onClickTab, selectedDetailsTab }) => {


Not sure I understand what is happening here... why this change? Seems like new functionality for an I18n PR.

andrewballantyne · 2020-11-26T19:56:25Z

Add page looks translated...

But we have actions that don't look translated:

Sidebar has a lot of non-translated chunks... not sure if that's a topology package issue with the new code location 🤔

All the actions but the two layouts look uncaught by I18n...

Partially missing translation:

Not sure who covers the build decorator 🤔 This is in the Topology package... so I assume this PR...

christianvogt

Overall I question the use of passing around t over simply using the global translation function when the translation of errors isn't reactive anyways.

christianvogt · 2020-11-26T19:49:27Z

frontend/packages/console-plugin-sdk/src/typings/post-form-submit.ts


 namespace ExtensionProperties {
  export interface PostFormSubmissionAction<T, P> {
    /** action type */
    type: string;
    /** callback for the related action */
-    callback: (arg: T, payload?: P) => Promise<T>;
+    callback: (t: TFunction, arg: T, payload?: P) => Promise<T>;


This isn't necessary. It seems like the only reason for translation is related to error messages.
These error messages can be translated using the global i18next object because there's no way these will be reactive anyways to a change in locale. We wouldn't be running the callback again with the new lang so therefore it's a one time translation.

christianvogt · 2020-11-26T19:52:47Z

frontend/packages/topology/src/actions/edgeActions.ts

  const resourceObj = getResource(edge.getSource());
  const resourceModel = modelFor(referenceFor(resourceObj));

  return {
-    label: 'Move Connector',
+    label: t('topology~Move Connector'),


You should use labelKey for actions.

christianvogt · 2020-11-26T19:53:19Z

frontend/packages/topology/src/actions/edgeActions.ts

    callback: () => {
-      moveConnectionModal({ edge, availableTargets });
+      moveConnectionModal({ edge, availableTargets, t: (x) => x });


Unnecessary to pass t here because the modal has access to useTranslation or withTranslation.

christianvogt · 2020-11-26T19:53:32Z

frontend/packages/topology/src/actions/edgeActions.ts

@@ -15,33 +16,33 @@ import { TYPE_CONNECTS_TO, TYPE_SERVICE_BINDING, TYPE_TRAFFIC_CONNECTOR } from '
 import { moveConnectionModal } from '../components/modals/MoveConnectionModal';
 import { removeConnection } from '../utils';

-const moveConnection = (edge: Edge, availableTargets: Node[]) => {
+const moveConnection = (edge: Edge, availableTargets: Node[], t: TFunction) => {


You should use labelKey for actions.
Same comment for all actions.

christianvogt · 2020-11-26T20:00:27Z

frontend/packages/topology/src/utils/removeConnection.tsx

    submitDanger: true,
    executeFn: () => {
-      return removeTopologyResourceConnection(edge).catch((err) => {
+      return removeTopologyResourceConnection(t, edge).catch((err) => {


I'd prefer to have confirmModal accept a callback which is either executed in the render context so that the function can use useTranslation or it gets passed in the translation function. This would make it easier to use these modal launchers without the need for the consumer to provide it's own t function.

christianvogt · 2020-11-26T20:09:26Z

frontend/packages/topology/src/utils/topology-utils.ts

@@ -97,6 +98,7 @@ export const getResourceKind = (node: Node): K8sResourceKindReference => {
 };

 export const updateTopologyResourceApplication = (
+  t: TFunction,


Again it seems we're passing around t to localize the return value of async operations that won't be re-run if the locale changes.
Seems unnecessary to pass t around everywhere for this and instead use the global i18next

christianvogt · 2020-11-26T20:16:40Z

frontend/packages/topology/src/components/page/TopologyPageToolbar.tsx

-            aria-label="Shortcuts"
-            bodyContent={TopologyShortcuts}
+            aria-label={t('topology~Shortcuts')}
+            bodyContent={getTopologyShortcuts(t)}


The API accepts a ReactNode. TopologyShortcuts can make use of useTranslation

Suggested change

bodyContent={getTopologyShortcuts(t)}

bodyContent={<TopologyShortcuts />}

christianvogt · 2020-11-26T20:17:32Z

frontend/packages/topology/src/components/modals/MoveConnectionModal.tsx

@@ -42,6 +45,7 @@ const MoveConnectionForm: React.FC<FormikProps<FormikValues> & MoveConnectionMod
  isSubmitting,
  cancel,
  values,
+  t,


Use useTranslation instead.

christianvogt · 2020-11-26T20:17:54Z

frontend/packages/topology/src/components/modals/EditApplicationModal.tsx

@@ -31,6 +34,7 @@ type EditApplicationModalProps = EditApplicationFormProps & {
 };

 const EditApplicationForm: React.FC<FormikProps<FormikValues> & EditApplicationFormProps> = ({
+  t,


Use useTranslation instead.

christianvogt · 2020-11-26T20:18:41Z

frontend/packages/topology/src/components/graph-view/components/nodeContextMenu.tsx

@@ -75,7 +76,7 @@ export const groupContextMenu = (element: Node, connectorSource?: Node) => {

  const graphData = element.getGraph().getData();
  return createMenuItems(
-    kebabOptionsToMenu(groupActions(graphData, applicationData, connectorSource)),
+    kebabOptionsToMenu(groupActions(t, graphData, applicationData, connectorSource)),


Actions can use labelKey

jeff-phillips-18 · 2020-12-01T11:33:54Z

@andrewballantyne Side bar is part of public/components/overview (not topology). I believe I have addressed other comments.

vikram-raj · 2020-12-01T14:11:33Z

/retest

vikram-raj · 2020-12-01T14:16:44Z

frontend/packages/topology/src/filters/TopologyFilterBar.tsx

+<<<<<<< HEAD
  const { filters, setTopologyFilters: onFiltersChange } = React.useContext(FilterContext);
+=======
+  const { t } = useTranslation();
+>>>>>>> Add i18n to topology package


@jeff-phillips-18 Missed this Merge conflict marker encountered.

andrewballantyne

Not sure I misunderstand Trans vs t ... but I think you can use t for all strings, and Trans when React components or DOM elements get in the way

andrewballantyne · 2020-12-01T15:56:23Z

frontend/packages/topology/src/filters/TopologyFilterBar.tsx

-                    Search results may appear outside of the visible area.{' '}
+                  <Trans ns="topology">
+                    Search results may appear outside of the visible area.


I think we still need the space 🤔

andrewballantyne · 2020-12-01T16:12:35Z

frontend/packages/topology/src/utils/moveNodeToGroup.tsx

+    const title = targetGroup ? (
+      <Trans ns="topology">Move Component Node</Trans>
+    ) : (
+      <Trans ns="topology">Remove Component Node from Application</Trans>


Don't think you need to use Trans on static strings 🤔

andrewballantyne · 2020-12-01T16:12:44Z

frontend/packages/topology/src/utils/moveNodeToGroup.tsx

+    const btnText = targetGroup ? (
+      <Trans ns="topology">Move</Trans>
+    ) : (
+      <Trans ns="topology">Remove</Trans>


andrewballantyne · 2020-12-01T16:13:03Z

frontend/packages/topology/src/utils/removeConnection.tsx

+      <Trans ns="topology">
+        Deleting the visual connector removes the `connects-to` annotation from the resources. Are
+        you sure you want to delete the visual connector?
+      </Trans>


andrewballantyne · 2020-12-01T16:13:06Z

frontend/packages/topology/src/utils/removeConnection.tsx

    </p>
  );

  return confirmModal({
    title: (
      <>
-        <YellowExclamationTriangleIcon className="co-icon-space-r" /> Delete Connector?
+        <YellowExclamationTriangleIcon className="co-icon-space-r" />{' '}
+        <Trans ns="topology">Delete Connector?</Trans>


andrewballantyne · 2020-12-01T16:13:11Z

frontend/packages/topology/src/utils/removeConnection.tsx

      </>
    ),
    message,
-    btnText: 'Delete',
+    btnText: <Trans ns="topology">Delete</Trans>,


andrewballantyne · 2020-12-01T16:31:18Z

frontend/packages/topology/src/components/side-bar/TopologyEdgeResourcesPanel.tsx

@@ -17,6 +18,7 @@ type TopologyEdgeResourcesPanelProps = {
 };

 const TopologyEdgeResourcesPanel: React.FC<TopologyEdgeResourcesPanelProps> = ({ edge }) => {


Missed Connections string passed to SidebarSectionHeading on line 36.

Oooh, missed the title missing.

With the pseudo lang:

With the "default" lang:

vikram-raj

missing header of the connector sidepanel

missing key warning

vikram-raj · 2020-12-01T15:32:38Z

frontend/packages/console-app/src/actions/modify-health-checks.ts

@@ -38,7 +39,8 @@ export const AddHealthChecks = (model: K8sKind, obj: K8sResourceKind): KebabOpti

 export const EditHealthChecks = (model: K8sKind, obj: K8sResourceKind): KebabOption => {
  return {
-    label: 'Edit Health Checks',
+    // t('console-app~Edit Health Checks')
+    labelKey: 'Edit Health Checks',


Suggested change

labelKey: 'Edit Health Checks',

labelKey: 'console-app~Edit Health Checks',

vikram-raj · 2020-12-01T16:54:45Z

frontend/packages/topology/src/components/list-view/cells/StatusCell.tsx

-      {ready} of {desired} pods
-    </Link>
-  );
+  return <Link to={href}>{t('topology~{{ready}} of {{desired}} pods', { ready, desired })}</Link>;


Suggested change

return <Link to={href}>{t('topology~{{ready}} of {{desired}} pods', { ready, desired })}</Link>;

return <Link to={href}>{t('topology~{{ready, number}} of {{count, number}} pod', { ready, count: desired })}</Link>;

jeff-phillips-18 · 2020-12-01T17:55:07Z

@andrewballantyne @vikram-raj Updated per comments.

vikram-raj · 2020-12-01T18:21:01Z

frontend/packages/topology/src/components/list-view/cells/StatusCell.tsx

@@ -50,11 +53,11 @@ const StatusCellResourceStatus: React.FC<StatusCellResourceStatus> = ({ obj, pod
    if (!filteredPods.length) {
      return null;
    }
-    return <Link to={href}>{filteredPods.length} pods</Link>;
+    return <Link to={href}>{t('topology~{{length}} pods', { length: filteredPods.length })}</Link>;


Suggested change

return <Link to={href}>{t('topology~{{length}} pods', { length: filteredPods.length })}</Link>;

return <Link to={href}>{t('topology~{{count, number}} pod', { count: filteredPods.length })}</Link>;

andrewballantyne

/lgtm

christianvogt · 2020-12-01T19:44:32Z

/approve

andrewballantyne · 2020-12-01T22:29:52Z

/lgtm

openshift-ci-robot · 2020-12-01T22:30:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, christianvogt, jeff-phillips-18

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:

~~frontend/OWNERS~~ [christianvogt]

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

openshift-ci-robot added the component/knative Related to knative-plugin label Nov 24, 2020

openshift-ci-robot requested review from christianvogt and jcaianirh November 24, 2020 15:50

openshift-ci-robot added component/kubevirt Related to kubevirt-plugin component/sdk Related to console-plugin-sdk component/shared Related to console-shared labels Nov 24, 2020

andrewballantyne suggested changes Nov 26, 2020

View reviewed changes

christianvogt reviewed Nov 26, 2020

View reviewed changes

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 27, 2020

jeff-phillips-18 force-pushed the topology-i18n branch from dd486e1 to ef896bb Compare November 30, 2020 21:08

jeff-phillips-18 force-pushed the topology-i18n branch from ef896bb to 4373009 Compare December 1, 2020 12:15

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 1, 2020

vikram-raj reviewed Dec 1, 2020

View reviewed changes

jeff-phillips-18 force-pushed the topology-i18n branch from 4373009 to 6f69e98 Compare December 1, 2020 15:06

andrewballantyne suggested changes Dec 1, 2020

View reviewed changes

andrewballantyne reviewed Dec 1, 2020

View reviewed changes

vikram-raj reviewed Dec 1, 2020

View reviewed changes

jeff-phillips-18 force-pushed the topology-i18n branch from 6f69e98 to 36e3932 Compare December 1, 2020 17:53

vikram-raj reviewed Dec 1, 2020

View reviewed changes

andrewballantyne approved these changes Dec 1, 2020

View reviewed changes

openshift-ci-robot assigned andrewballantyne Dec 1, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 1, 2020

openshift-ci-robot assigned christianvogt Dec 1, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 1, 2020

Add i18n to topology package

e60f94e

jeff-phillips-18 force-pushed the topology-i18n branch from 36e3932 to e60f94e Compare December 1, 2020 22:29

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 1, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 1, 2020

openshift-merge-robot merged commit 838da0c into openshift:master Dec 2, 2020

jeff-phillips-18 deleted the topology-i18n branch December 2, 2020 13:32

spadgett added this to the v4.7 milestone Dec 9, 2020

spadgett added the kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated label Jan 8, 2021

	bodyContent={getTopologyShortcuts(t)}
	bodyContent={<TopologyShortcuts />}

		@@ -17,6 +18,7 @@ type TopologyEdgeResourcesPanelProps = {
		};

		const TopologyEdgeResourcesPanel: React.FC<TopologyEdgeResourcesPanelProps> = ({ edge }) => {

	labelKey: 'Edit Health Checks',
	labelKey: 'console-app~Edit Health Checks',

	return <Link to={href}>{t('topology~{{ready}} of {{desired}} pods', { ready, desired })}</Link>;
	return <Link to={href}>{t('topology~{{ready, number}} of {{count, number}} pod', { ready, count: desired })}</Link>;

	return <Link to={href}>{t('topology~{{length}} pods', { length: filteredPods.length })}</Link>;
	return <Link to={href}>{t('topology~{{count, number}} pod', { count: filteredPods.length })}</Link>;

Add i18n to topology package #7315

Add i18n to topology package #7315

Conversation

jeff-phillips-18 commented Nov 24, 2020

jeff-phillips-18 commented Nov 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewballantyne commented Nov 26, 2020

christianvogt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeff-phillips-18 commented Dec 1, 2020

vikram-raj commented Dec 1, 2020

Choose a reason for hiding this comment

andrewballantyne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewballantyne Dec 1, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vikram-raj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeff-phillips-18 commented Dec 1, 2020

Choose a reason for hiding this comment

andrewballantyne left a comment

Choose a reason for hiding this comment

christianvogt commented Dec 1, 2020

andrewballantyne commented Dec 1, 2020

openshift-ci-robot commented Dec 1, 2020

andrewballantyne Dec 1, 2020 •

edited