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

React-intl: Add i18n to SpeedRunEthereum #187

Open
wants to merge 26 commits into
base: master
Choose a base branch
from
Open

React-intl: Add i18n to SpeedRunEthereum #187

wants to merge 26 commits into from

Conversation

dgrcode
Copy link
Collaborator

@dgrcode dgrcode commented Mar 31, 2023

There are two main ways to add internationalized messages:

Inside JSX (docs)

Most of the time this is what we need. The way to use <FormattedMessage /> with rich text:

<FormattedMessage
  id="message.id"
  defaultMessage="Hello <big>{name}</big>"
  values={{
    name: 'Foo',
    big: chunks => <h1>{chunks}</h1>
  }}

Note there are two things to interpolate: the things in {}, and the things in <>.

Imperative API (docs)

When the thing to be translated is not a react component, we need to use the imperative API. Here are the useIntl hook docs, and the Message Descriptor API docs. Example:

const intl = useIntl()

return (
  <span aria-label={intl.formatMessage({ id: "signatureSingUp.write-icon", defaultMessage: "write icon" })}></span>
)

Gotchas

Missing values

Something I found out is that a missing value will show the default message. For example:

<FormattedMessage
  id="message.id"
  defaultMessage="Hello {name}"
  values={{
    name: 'Foo',
  }}

and en.json:

{
  "message.id": "Hello <h1>{name}</h1>"
}

will show the default message because it doesn't know how to resolve the h1 reference.

Value names with dashes

Something like this wouldn't work:

<FormattedMessage
  id="message.id"
  defaultMessage="Hello {icon-heart}"
  values={{
    "icon-heart": <HeartFilled />,
  }}

But naming it {iconHeart} instead, does work

Vertical spaces

Vertical spaces in the defaultMessage will be converted into a single horizontal space in the en.json file when extracting the mesages. Keep an eye for things like:

<FormattedMessage
  id='foo'
  defaultMessage=`Click the
    <Link>
      documentation
    </Link>
  . Read the docs there.`
  values={{...}}
/>

// in en.json becomes
{
  "foo": "Click the <Link> documentation </Link> . Read the docs there."
}

Tag arguments

Note how the <Link> in the previous example doesn't have arguments. That's because the message extractor parser will skip the arguments (read more in the docs). The proper way to do it is to give it the arguments you want in the value property. Like this:

<FormattedMessage
  id='foo'
  defaultMessage=`Click the <Link>documentation</Link>.`
  values={{
    Link: chunks => (
      <Link href='/page' fontWeight="700" color="teal.500" isExternal>
        {chunks}
      </Link>
    )
  }}
/>

Self closing tags

Due to the same reason tag arguments don't work, self closing tags aren't supported either (read more in the docs). The way to solve it is with interpolations:

<FormattedMessage
  id='foo'
  defaultMessage=`Line one{br}Line two`
  values={{
    br: <br />
  }}
/>

@netlify
Copy link

netlify bot commented Mar 31, 2023

Deploy Preview for wonderful-kirch-4ab41a ready!

Name Link
🔨 Latest commit 8b8f395
🔍 Latest deploy log https://app.netlify.com/sites/wonderful-kirch-4ab41a/deploys/645d54fec87c160008b91bc2
😎 Deploy Preview https://deploy-preview-187--wonderful-kirch-4ab41a.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@dgrcode dgrcode changed the title Re-do commit 583ef70 on current master React-intl: Re-do commit 583ef70 on current master Apr 3, 2023
@dgrcode
Copy link
Collaborator Author

dgrcode commented Apr 3, 2023

I'd go through each component and replace plain texts with the proper <FormatMessage /> components.

A bit more tricky can be to find strings outside of the returned jsx. I'd look for aria-label in the whole repo. Calls to toast or useToast can also be tricky. In that case, both title and description props accept react components (source), so we can use FormatMessage there as well EDIT: That doesn't work as expected (read this comment below, but the imperative API works fine.

Any other ideas where to find those strings?

@dgrcode dgrcode requested a review from carletex April 3, 2023 20:19
@carletex
Copy link
Collaborator

carletex commented Apr 4, 2023

Great work @dgrcode!!

A bit more tricky can be to find strings outside of the returned jsx. I'd look for aria-label in the whole repo. Calls to toast or useToast can also be tricky. In that case, both title and description props accept react components, so we can use FormatMessage there as well.

Toasts are important, but don't worry too much about other details like "aria-label" (at least for now)

Main components/pages are the HomeView, BuilderProfileView, ChallengeDetailView (even tho the README will come from Github). You can ignore SubmissionReviewView.

We will also need to take care of packages/react-app/src/data/challenges.js, which is used for display purposes (label + description. Branch could be dynamic like base-branch-{langcode}). What are the options? Off the top of my head:

  1. Have individual challenges_{langcode}.js files. We'll duplicate a bunch of data.
  2. Since it's a js file (and not json) we can do the i18n there, and treat it like a regular component file?

To send to the EF translators.

  • SRE front-end en.json file (we'll be able to generate it with this PR)
  • All the READMEs from the challenges (Nothing to do in this PR)
  • Autograder response. This is for another PR (even repo :)). But just noting it here: https://github.com/austintgriffith/speedrun-grader

Next steps (future PRs?)

We also need to do all the language selection logic: the selector itself, the routing (langcode suffix on the path? /en /es). But we prolly want to merge this PR and work on the language selection logic in another PR.

Thanks!

@dgrcode
Copy link
Collaborator Author

dgrcode commented Apr 16, 2023

Regarding packages/react-app/src/data/challenges.js, I guess the easiest would be to have different files, one for each language (your option 1). The only problem I see with that is that translators might not be confortable creating .js files, and might prefer just touching {lang}.json files?

I'm going through the files you mentioned and the components imported from them. I'll be updating here as I progress:

  • HomeView
    • ChallengeExpandedCard. I haven't touched label defined in line 67. Is it needed?
      • Join BG
        • Note on this one. When the signature fails after signing with the wallet, the server might respond with an error message using error.response.data. In that case, we show that message to the user. We'd need to translate that one on the server. Is it needed?
  • BuilderProfileView
    • BuilderProfileCard
      • Note: I haven't done i18n on handleUpdateReachedOutFlag or on content under isAdmin
    • JoinedBuidlGuidlBanner
    • BuilderProfileHeader
      • I haven't translated the role names "frontend", "backend", etc, defined in userFunctionDescription
    • BuilderCallenges
      • ChallengeStatusTag
  • ChallengeDetailView
    • ChallengeSubmission

Notes & open questions:

  • Tinkering with users, logins, registrations and so on, I've found a few error messages that I haven't gone through yet, but it might be useful doing a global search with the term Couldn't. I've seen a few messages that we might want to translate. Leaving it here for now, as I might translate them going through the components in the list above.
  • There are some places where the logic to show some content depends on the user being an admin. I think those aren't necessary to translate, so I'll leave them untouched unless they're needed. No need.
  • Similar to challenges.js, we might want to do i18n with socials.js. The only text needing i18n there are the placeholders, so we might want to skip it. -> Translate with imperative API
  • Just to double check with you that's ok to leave the role names in English (at react-app/src/helpers/constants.js) It's ok.
  • Do we want to do i18n on date formats? I saw DateWithTooltip It's ok

@dgrcode
Copy link
Collaborator Author

dgrcode commented Apr 17, 2023

Trying to use <FormattedMessage /> as a Toast title gave me problems. It complained about <IntlProvider /> not being in the component ancestry. I imagine this happens because Toast is internally using a Portal or something similar that moves the final component out of the component hierarchy.

Anyways, the imperative solution with the useIntl hook works fine, so I'll use that method for the toasts.

@carletex
Copy link
Collaborator

Regarding packages/react-app/src/data/challenges.js, I guess the easiest would be to have different files, one for each language (your option 1). The only problem I see with that is that translators might not be confortable creating .js files, and might prefer just touching {lang}.json files?

I think we should have one file and use the imperative solution there, since the object key, id, disabled, previewImage, dependencies, (even branch-name if we dynamically load the READMEs, appending the langcode). Making a change to any of these fields will imply tweaking all of them.

Not sure if import { challengeInfo } from "../data/challenges"; will automagically pick the i18n context and give you the right translation. If that's the case, let's 100% go with a single file. If not, let's rethink the options.

Note on this one. When the signature fails after signing with the wallet, the server might respond with an error message using error.response.data. In that case, we show that message to the user. We'd need to translate that one on the server. Is it needed?

If this is the only instance where we'd need to translate server stuff, we can skip it, to avoid setting up all the i18n stuff in the server. If I remember correctly, all the auto-grading feedback comes from https://github.com/austintgriffith/speedrun-grader. At some point, we'd need to implement server i18n there.

@dgrcode
Copy link
Collaborator Author

dgrcode commented Apr 17, 2023

Re: packages/react-app/src/data/challenges.js, sounds good. I'll see how it gets the context. Worst case we can initialize it somehow like adding a method like setChallengesLang that would set a constant in that file with the language, so we can pass it to intl.

As for the server i18n, so far that's the only instance I've found. I'll continue updating this comment as I progress. We can use that one as a summary, and I'll add a new comment on the PR when I update it so you know there have been progress.

@dgrcode
Copy link
Collaborator Author

dgrcode commented Apr 17, 2023

Added more texts and two notes:

  • There are some places where the logic to show some content depends on the user being an admin. I think those aren't necessary to translate, so I'll leave them untouched unless they're needed.

  • Similar to challenges.js, we might want to do i18n with socials.js. The only text needing i18n there are the placeholders, so we might want to skip

@dgrcode
Copy link
Collaborator Author

dgrcode commented Apr 19, 2023

Added more texts and a notes:

  • Just to double check with you that's ok to leave the role names in English (at react-app/src/helpers/constants.js)

  • Do we want to do i18n on date formats? I saw DateWithTooltip

@carletex
Copy link
Collaborator

This is looking good @dgrcode, good job!!

There are some places where the logic to show some content depends on the user being an admin. I think those aren't necessary to translate, so I'll leave them untouched unless they're needed.

Yep, no need.

Similar to challenges.js, we might want to do i18n with socials.js. The only text needing i18n there are the placeholders, so we might want to skip it.

If the i18n context works correctly when importing the file, I'd use the imperative api to translate the placeholders, since it seems straight forward.

Just to double check with you that's ok to leave the role names in English (at react-app/src/helpers/constants.js)

Yep, English is fine I think.

Do we want to do i18n on date formats? I saw DateWithTooltip

I think YYYY-MM-DD, HH:mm is international enough :D Let's skip it.

Thanks Daniel!

@dgrcode
Copy link
Collaborator Author

dgrcode commented Apr 19, 2023

Awesome! Thanks! I'll update the comment with your responses to have it all in one place.

@dgrcode
Copy link
Collaborator Author

dgrcode commented Apr 19, 2023

Just realized while looking at the table, that the DateWithTooltip shows the relative date like "2 years ago". The YYYY-MM-DD, HH:mm is shown in the tooltip only.

There's a FormattedRelativeTime component, but from what I'm seeing in their examples they say at most "days". So "2 years ago" would become something like "700 days ago".

I think this is one of those things that is just not worth it translating.

@dgrcode
Copy link
Collaborator Author

dgrcode commented Apr 24, 2023

I added the last component that was remaining. I think this can be merged as a first iteration.

Next iteration would be to check how to use a specific language integrating it with the route like speedrunethereum.com/es/....

Also check how to do i18n with challenges.js and socials.js.

@dgrcode
Copy link
Collaborator Author

dgrcode commented Apr 24, 2023

@carletex I've lost the button to make this a PR instead of a draft. I can only see the option to close it or comment 🤷

Of course I found it as soon as I sent this message. I was looking for "Draft" or "PR", but the button was "Ready for review" almost looking like disabled 🤦

@dgrcode dgrcode marked this pull request as ready for review April 24, 2023 11:26
@carletex
Copy link
Collaborator

Looking great @dgrcode

Also check how to do i18n with challenges.js and socials.js.

Can we test this in this PR? If it's straightforward (i18n context works correctly when importing the file) let's include it here, so we can send the en.json file to the translators while we work on the routing.

Thanks!

@dgrcode
Copy link
Collaborator Author

dgrcode commented Apr 27, 2023

sure! I'll look into it

@dgrcode
Copy link
Collaborator Author

dgrcode commented Apr 27, 2023

The best solution I've found is to modify challenges.js to return a function getChallengeInfo instead of the challenges object. That function receives the intl object, so we can do stuff like:

getChallengeInfo = intl => ({
  "simple-nft-example": {
    label: intl.formatMessage({
      id: "challenges.simple-nft-example.label",
      defaultMessage: "🚩 Challenge 0: 🎟 Simple NFT Example",
    }),
    // ...
  }
})

I've tested it works. I'll continue adding formatMessage to the relevant fields. Just to double check, we only need translations for label and description, right?

@carletex
Copy link
Collaborator

I've tested it works.

Sounds good!

Just to double check, we only need translations for label and description, right?

Yes!

Thanks Dani

@dgrcode
Copy link
Collaborator Author

dgrcode commented Apr 28, 2023

I've added i18n to challenges.js. I've removed an example button I had in a separate commit 441875b so we have a reference of how to change the locale manually.

@carletex
Copy link
Collaborator

carletex commented May 1, 2023

Looking great @dgrcode thanks!

I sent the en.json files to the translators, to see if everything looks good for them.

Could you fix the warnings, Daniel? (check the react console - yarn start)

image


@technophile-04 could you help me test this? TLDR: We are preparing SRE to be translated, and this PR wraps every string with i18n functions/components. Translations are still not done, but we want to merge this PR (default to EN) so everything should be working as in main. While the translations are being worked, we'll be implementing the language selector / routes, so when we have all the files (es.json, hi.json, etc) we'd just add the files and enable the selector.

@technophile-04
Copy link
Member

This is really cool !!! Tysm for the TLDR; also read the whole discussion, will start testing it out 🙌

@dgrcode
Copy link
Collaborator Author

dgrcode commented May 2, 2023

woops! I'll look into the warnings and push a commit

@dgrcode
Copy link
Collaborator Author

dgrcode commented May 3, 2023

done, they should be fixed now!

@technophile-04
Copy link
Member

Just tested it out nicely really nice work @dgrcode !!!! I wasn't really able to find any big bugs/issues and functionality wise everything works 🚀.

Assumptions for test :

How did I test :

For testing I generated es.json and hi.json using this cli tool (its not perfect but for testing purpose its good) and then revied both code and UI

Test

  • Imperative API's works nicely like in challenge.js
  • Functinality wise also everthing works nicely.
  • Almost all the strings are converted to pass through react-intl leaving things mentioned in the "Assumption for test" section and I think we might have missed (not really sure on this though)
    • Navlinks
    • Footer ("Build with" part)

Thought :

This is not really needed and it just came to my mind instead passing defaultMessage everytime to FormattedMessage what if we
if we create an custom format message component like :

import  enLang  from  "../lang/en.json";
export  default  function  CustomFormattedMessage({  stringId,  string,  values  }) {
return (
	<FormattedMessage  id={stringId}  defaultMessage={enLang[stringId]}  values={values}>
		{string}
	</FormattedMessage>
  );
}

Since the default is message same as en. But hopefully it does not turns out bad abstraction so not too strong on this. Also yarn translations-extract will not work with this I think.

Also saw GH actions is failing due to node version issue, will create a PR around it

@carletex
Copy link
Collaborator

carletex commented May 8, 2023

Thanks for the review @technophile-04 !!

Navlinks
Footer ("Build with" part)

Yep, we definitely want to translate that too! cc @dgrcode

if we create an custom format message component like
Also yarn translations-extract will not work with this I think.

The process is creating the (with a default message) & then run the translations-extract, which will take the default message and adds it to the en.json file. So I don't think what you are proposing would work: I think it needs "static" strings to be able to extract.

@dgrcode
Copy link
Collaborator Author

dgrcode commented May 11, 2023

Yep, we definitely want to translate that too! cc @dgrcode

I'll add it to those places and push a commit!

@dgrcode
Copy link
Collaborator Author

dgrcode commented May 11, 2023

Something that crossed my mind as I was adding some <FormattedMessage />. Maybe we don't want to translate the term "builder" since it's kind of a community slang. I was thinking it might become translated as "constructor" in Spanish, which is correct by any dictionary, but it definitely has a different meaning.

What do you think @carletex?

@dgrcode
Copy link
Collaborator Author

dgrcode commented May 11, 2023

Something undocumented (as far as I could find): don't used dashed names for values. I updated the gotchas section.

Value names with dashes

Something like this wouldn't work:

<FormattedMessage
  id="message.id"
  defaultMessage="Hello {icon-heart}"
  values={{
    "icon-heart": <HeartFilled />,
  }}

But naming it {iconHeart} instead, does work

Comment on lines -20 to +36
<HStack alignItems="center">
<span>Built with</span> <HeartFilled /> <span>at</span>
</HStack>
<Link color={linkColor} href="https://buidlguidl.com/" isExternal>
BuidlGuidl
</Link>
<FormattedMessage
id="footer.built-with-love-at-buidlguidl"
defaultMessage="Built with {heartIcon} at <Link>BuidlGuidl</Link>"
values={{
heartIcon: (
<HStack alignItems="center" mx="5px">
<HeartFilled />
</HStack>
),
Link: chunks => (
<Link color={linkColor} href="https://buidlguidl.com/" isExternal>
{chunks}
</Link>
),
}}
/>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here I made a change to the html structure of the footer. I think the visual result is exactly the same, but we might want to double check it. The reason is that otherwise the translation string would have ended up being:

<HStack><span>Built with</span> {heartIcon} <span>at</span></Hstack><Link>BuidlGuidl</Link>

That html is complex enough that I don't think a typical translator could re-create it in case the order of the phrase changed in a different language.

@dgrcode dgrcode changed the title React-intl: Re-do commit 583ef70 on current master React-intl: Add i18n to SpeedRunEthereum May 21, 2023
@dgrcode dgrcode mentioned this pull request May 22, 2023
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.

3 participants