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

feedbacks #8

Open
fredericrous opened this issue Mar 8, 2024 · 5 comments
Open

feedbacks #8

fredericrous opened this issue Mar 8, 2024 · 5 comments

Comments

@fredericrous
Copy link

fredericrous commented Mar 8, 2024

hey! thank you for the template. Here are some feedbacks and config suggestions

lang in url

I would love to see the lang in the url for:

  • SEO purpose
  • simplicity for geek users that will just change the language from the url
  • being able to send a link to someone in a desired language

There's a very recent article on that on the remix blog. Have you seen it? https://remix.run/blog/remix-i18n
I'm thinking maybe to not have to prefix everything with ($lang) as it's suggested in part3 of the article, we could have a routes folder structure like

  • /routes/lang/
    • $lang.index.tsx
    • _auth+
    • _routes/
      • _marketing+
      • _seo+
      • ...
    • $.tsx
    • me.tsx

of course a search param like ?lang= is not an option for SEO

18next-parser configuration

I saw a reference in the readme to i18next-parser but no reference in the code to it. I added it to the dev dependencies and then added a script to package.json

    "i18n": "i18next -c i18next-parser.config.js",
//  i18next-parser.config.js
export default {
  keySeparator: false, // important to get a flat json format. hierarchical json does not seem to work as is
  input: ['app/**/*.{js,jsx,ts,tsx}'],
  output: './public/locales/$LOCALE/common.json'
}

also we could specify in readme that i18next-parser does not support react specific <Trans /> component

github action

nice to have final feature: a github action that checks there are no missing translation (some in the code but not in the translation json file)

unwanted redirect

Also maybe I missed something when I implemented epic-stack-with-i18n in my project but I get redirected to root when I switch lang

missing examples of translation

  • it would be nice to see an example or a mention in the readme on how to translate the page title set by export const meta: MetaFunction
  • same idea with emails
  • same idea with tests, that way we can use the translation in the test instead of duplicating the text

example to setup e2e tests with translations

  • set the local in playwright config, as described in this article
  • set a fixture like
// playwright-utils.ts
import commonJson from '#public/locales/en/common.json' assert { type: "json" }
...

export const test = base.extend<{
  insertNewUser(options?: GetOrInsertUserOptions): Promise<User>
  login(options?: GetOrInsertUserOptions): Promise<User>
  i18next(options?: i18nCallback): Promise<TFunction<"common", undefined>>
}>({
  ...
  i18next:  async ({}, use) => {
    await use(async options => {
      const instance = i18next.createInstance();
      const i18nInit = instance.init({ns: ['en'], ...i18nOptions, ...options});
      // instance.addResourceBundle('en', 'common', commonJson, true) -- not necessary with PR#9
      return i18nInit;
    });
  }
  ...
}
``

we would use this fixture in tests like

```ts
test('Users can add 2FA to their account and use it when logging in', async ({
  page,
  login,
  i18next,
}) => {
  const t = await i18next();
  ...
  await expect(main).toHaveText(new RegExp(t('auth.2fa.enabled'), 'i'))
  ...
}

example to setup testing-library tests with translations

// setup-test-env.ts
export const i18nextInstance = i18next.createInstance();
await i18next.use(initReactI18next).init({ ns: ['en'], ...i18nOptions })

when I run the tests, they pass as is, I18nextProvider doesn't seem needed

Final word

thank you for the template. Keep up the good work.
You can close the issue when you read, or let it open for others to give feedback

@rperon
Copy link
Owner

rperon commented Mar 8, 2024

Wow, first thank you for this much appreciated feedback !

Yes I read the recent article in the remix blog and it's a good summary of how to do i18n with remix.

lang in url

For the lang in the url, the main reason why I didn't do it is that I don't like to have everything prefixed by an optional segment. That being said, what you suggest can be done but it might be interesting to see if we can do something similar to what NextJS do with i18n routing : https://nextjs.org/docs/app/building-your-application/routing/internationalization#routing-overview

Maybe with vite, we can figure out something so that we don't have to prefix everything even if remix-flat-routes is a big help.

18next-parser configuration

I think it got deleted when I move the example to epic-stack with remix-v2. Here is the PR #10 where It's back.
I also have another PR, which might put this one obsolete, but I'm not ready to merge it yet if you want to check it out #9

github action

Very good idea, I created a separate issue to track a solution #11

unwanted redirect

This is a bug, it should not behave that way. Here is the PR #12 with the fix.

missing examples of translation

You are right, meta translation should be easy.
For email, it might get a little bit trickier.

Very nice solution for playwright I will give it a try.

@fredericrous
Copy link
Author

fredericrous commented Mar 8, 2024

thank you for the PRs and for the creation of the issues. I added a review on #10 I have some refacto to do on my code now hahaha

Also 18next-parser does not work with dynamic keys t(`my-dynamic-${key}`), but they suggest a workaround on their readme https://github.com/i18next/i18next-parser. it could be worth documenting in epic-stack-with-i18n readme

@rperon
Copy link
Owner

rperon commented Mar 8, 2024

Your welcome 👍

Yes for dynamic key, the recommended way to do that is with comments: https://github.com/i18next/i18next-parser?tab=readme-ov-file#caveats .

I need to look for a dynamic key inside the stack so it can be a good example, will add to the readme.

update: it's already on the readme inside the 22/09/2023 update

@fredericrous
Copy link
Author

I added an example on how to setup i18next with testing-library ☝️

@rperon
Copy link
Owner

rperon commented Mar 10, 2024

I've created a first PR for a playwright implementation #13 .
The nice thing with this example, is that each test can be tested with it's translation !

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

No branches or pull requests

2 participants