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

Room UI Redesign: Auth Localization #3555

Merged
merged 6 commits into from
Dec 15, 2020

Conversation

robertlong
Copy link
Contributor

@robertlong robertlong commented Dec 14, 2020

This is the first of the localization PRs. I'm going to split them up into chunks around the same size as this throughout the week. This first PR covers the authentication components, including sign-in modal messages used throughout the client.

I'm establishing a slightly new workflow and a few best practices for localization based off the react-intl documentation, a blog post by the team at Calm, and the work needed to integrate with Mozilla Pontoon.

The new workflow will rely on react-intl's CLI tool for message extraction. We will define an id, a default message, and optionally a description as we already do with the <FormattedMessage> component or with the defineMessage() method. This data will be extracted along with the file path and line numbers and it will be fed into Pontoon. From there, Pontoon will be used to generate translation files. Which will be committed back to the Hubs repository by Pontoon's bot. These files will be in a new format from the existing files. At build time we will translate them into the react-intl compatible files.

The best practices for localization are the following:

1. Message ids cannot contain variables.

This is due to the message extraction relying on static code analysis.

Example:

import { FormattedMessage, defineMessages, useIntl } from "react-intl";

// Good
<FormattedMessage id="sign-in-modal.complete" defaultMessage="Sign in complete" />

// Bad
<FormattedMessage id={`sign-in-modal.${step}`} />

// Good
const messages = defineMessages({
  prompt: {
    id: "sign-in-modal.prompt",
    defaultMessage: "Please sign in"
  },
  complete: {
    id: "sign-in-modal.complete",
    defaultMessage: "Sign in complete"
  }
});

function SignInModal({ step }) {
  const intl = useIntl();

  return <p>{intl.formatMessage(messages[step])}</p>;
}

// Bad
function SignInModal({ step }) {
  return <p><FormattedMessage id={`sign-in-modal.${step}`} /></p>;
}

2. Message ids should be namespaced with the component/parent component name.

We are not using auto-generated ids due to backwards-compatibility issues with our existing translations. As such, we need to ensure that there are no collisions. Name the translation ids by starting with the component name in dash/kebab case. If the component is split up into multiple local components and is only used in the context of a parent component, use the parent component name.

Example:

// Good
function SignInModal() {
 return <FormattedMessage id="sign-in-modal.complete" defaultMessage="Sign in complete" />;
}

// Bad
function SignInModal() {
 return <FormattedMessage id="sign-in.complete" defaultMessage="Sign in complete" />;
}

// Good
function ObjectListItem({ item }) {
 return (
  <li>
    {item.name}
    <button>
      <FormattedMessage id="object-list.select" defaultMessage="Select Object" />
    </button>
  </li>
  );
}

export function ObjectList({ objects }) {
 return <ul>{objects.map(o => <ObjectListItem item={o} />)}</ul>;
}

3. Always specify a default message.

Default messages will be used to provide context to translators, they are the fallback message if it is not defined for another language, and they are used as the English translation by default.

// Good
<FormattedMessage id="sign-in-modal.complete" defaultMessage="Sign in complete" />

// Bad
<FormattedMessage id={`sign-in-modal.complete`} />

// Good
const signInCompleteMessage = defineMessage({
  id: "sign-in-modal.complete",
  defaultMessage: "Sign in complete"
});

// Bad
const signInCompleteMessage = defineMessage({
  id: "sign-in-modal.complete"
});

4. All strings presented to users should be localized including accessibility features

It is not enough to localize just the visible text. All labels, alt text, and callouts should also be localized.

<img alt={<FormattedMessage id="logo.alt-text" defaultMessage="Hubs Logo" />} src="logo.png" />

<div aria-label={<FormattedMessage id="element.label" defaultMessage="This is a label" />}></div>

<div title={<FormattedMessage id="element.title" defaultMessage="This is a title" />}></div>

5. Use react-intl's formatter for formatting concatenated strings.

Different languages have different grammatical rules and sentence structure. Relying on string concatenation that works in English will not always make sense in another language. Use react-intl's values prop to format messages with variables or rich formatting. If formatting gets too complicated break it up into multiple messages.

// Bad
<p>
  <small>
    By proceeding, you agree to the{" "}
    {showTerms && (
      <>
        <a rel="noopener noreferrer" target="_blank" href={termsUrl}>
          terms of use
        </a>{" "}
      </>
    )}
    {showTerms && showPrivacy && "and "}
    {showPrivacy && (
      <a rel="noopener noreferrer" target="_blank" href={privacyUrl}>
        privacy notice
      </a>
    )}.
  </small>
</p>
// Good
export function LegalMessage({ termsUrl, privacyUrl }) {
  const toslink = useCallback(
    chunks => (
      <a rel="noopener noreferrer" target="_blank" href={termsUrl}>
        {chunks}
      </a>
    ),
    [termsUrl]
  );

  const privacylink = useCallback(
    chunks => (
      <a rel="noopener noreferrer" target="_blank" href={privacyUrl}>
        {chunks}
      </a>
    ),
    [privacyUrl]
  );

  if (termsUrl && privacyUrl) {
    return (
      <FormattedMessage
        id="legal-message.tos-and-privacy"
        defaultMessage="By proceeding, you agree to the <toslink>terms of use</toslink> and <privacylink>privacy notice</privacylink>."
        values={{
          toslink,
          privacylink
        }}
      />
    );
  }

  if (termsUrl && !privacyUrl) {
    return (
      <FormattedMessage
        id="legal-message.tos"
        defaultMessage="By proceeding, you agree to the <toslink>terms of use</toslink>."
        values={{
          toslink
        }}
      />
    );
  }

  if (!termsUrl && privacyUrl) {
    return (
      <FormattedMessage
        id="legal-message.privacy"
        defaultMessage="By proceeding, you agree to the <privacylink>privacy notice</privacylink>."
        values={{
          privacylink
        }}
      />
    );
  }

  return null;
}
  1. Reference messages as objects or defineMessages keys, not ids
    Message objects contain an id and a default message. Pass the whole object into a component and use intl.formatMessage to render it. Alternately, use a defineMessages function to define messages with keys and use that key to reference the message.
const errorMessages = defineMessages({
  unknown: {
    id: "error-message.unknown",
    defaultMessage: "An unknown error occured"
  },
  notFound: {
    id: "error-message.not-found",
    defaultMessage: "Page not found"
  }
});

// Good
function ErrorMessage({ message }) {
  const intl = useIntl();

  return <code>{intl.formatMessage(message)}</code>
}

<ErrorMessage message={errorMessages.unknown} />

// Good
function ErrorMessage({ type }) {
  const intl = useIntl();

  return <code>{intl.formatMessage(errorMessages[type])}</code>
}

<ErrorMessage type="unknown" />

I've installed Calm's linter rules that will check for some of these issues. Right now we are failing a lot of those rules and there are a lot of warnings. I'll work through these issues throughout the week in separate PRs until they are corrected. Once they are corrected I will switch the linter to throw errors rather than warn.

Copy link
Contributor

@robinkwilson robinkwilson left a comment

Choose a reason for hiding this comment

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

A few small things. Looks good. So excited for this localization linter update!

// Good
export function LegalMessage({ termsUrl, privacyUrl }) {
const toslink = useCallback(
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
const toslink = useCallback(
const tosLink = useCallback(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the capitalization here is correct as lowercase since it's being used as an HTML element name.

Notice how it's used here:

<FormattedMessage
  id="legal-message.privacy"
  defaultMessage="By proceeding, you agree to the <privacylink>privacy notice</privacylink>."
  values={{
    privacylink
  }}
/>

);

const privacylink = useCallback(
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
const privacylink = useCallback(
const privacyLink = useCallback(

import { FormattedMessage } from "react-intl";

export function LegalMessage({ termsUrl, privacyUrl }) {
const toslink = useCallback(
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
const toslink = useCallback(
const tosLink = useCallback(

[termsUrl]
);

const privacylink = useCallback(
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
const privacylink = useCallback(
const privacyLink = useCallback(

id="legal-message.tos-and-privacy"
defaultMessage="By proceeding, you agree to the <toslink>terms of use</toslink> and <privacylink>privacy notice</privacylink>."
values={{
toslink,
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
toslink,
tosLink,

}
/>
{this.state.failedAtLeastOnce ? (
<FormattedMessage id="link.try_again" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be link-root.try_again since the component is called LinkRoot?

Suggested change
<FormattedMessage id="link.try_again" />
<FormattedMessage id="link-root.try_again" />

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did all of the non-auth component changes just to get tests to pass, but yeah I need to come back and fix these.

{this.state.failedAtLeastOnce ? (
<FormattedMessage id="link.try_again" />
) : this.state.isAlphaMode ? (
<FormattedMessage id="link.link_page_header_headset" />
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
<FormattedMessage id="link.link_page_header_headset" />
<FormattedMessage id="link-root.link_page_header_headset" />

) : this.state.isAlphaMode ? (
<FormattedMessage id="link.link_page_header_headset" />
) : (
<FormattedMessage id="link.link_page_header_entry" />
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
<FormattedMessage id="link.link_page_header_entry" />
<FormattedMessage id="link-root.link_page_header_entry" />

import { Tip } from "./Tip";
import { useEffect } from "react";
import { discordBridgesForPresences, hasEmbedPresences } from "../../utils/phoenix-utils";

const onboardingMessages = defineMessages({
"tips.mobile.look": {
id: "tips.mobile.look",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should all of these be tip-container rather than tips ?

Suggested change
id: "tips.mobile.look",
id: "tip-container.mobile.look",

export function SubmitEmail({ onSubmitEmail, initialEmail, showPrivacy, privacyUrl, showTerms, termsUrl, message }) {
export const SignInMessages = defineMessages({
pin: {
id: "sign-in.pin",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these be sign-in-modal (and update the en.json, etc)? I believe this is your exact example in the documentation.

Suggested change
id: "sign-in.pin",
id: "sign-in-modal.pin",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup 😅 I meant to come back and fix this, thanks for catching it!

@robertlong robertlong merged commit 180161f into room-ui-redesign Dec 15, 2020
@robertlong robertlong deleted the redesign/auth-localization branch December 15, 2020 21:08
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.

2 participants