vendure-ecommerce / storefront-remix-starter

A storefront starter kit for Vendure built with Remix
https://remix-storefront.vendure.io
188 stars 105 forks source link

Remix i18n #57

Closed jeancx closed 9 months ago

jeancx commented 10 months ago

56

Done

Ready for review.

image

This site helps a lote to generate translations:

https://translate.i18next.com/

Info:

netlify[bot] commented 10 months ago

Deploy Preview for inspiring-boba-355f4d ready!

Name Link
Latest commit dd57c253bfc84b1a7a126bea1052ea05eebf9869
Latest deploy log https://app.netlify.com/sites/inspiring-boba-355f4d/deploys/657af95a7307d100089feafa
Deploy Preview https://deploy-preview-57--inspiring-boba-355f4d.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 configuration.

netlify[bot] commented 10 months ago

Deploy Preview for remix-vendure ready!

Name Link
Latest commit dd57c253bfc84b1a7a126bea1052ea05eebf9869
Latest deploy log https://app.netlify.com/sites/remix-vendure/deploys/657af95a4e73f30008907d92
Deploy Preview https://deploy-preview-57--remix-vendure.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 configuration.

kyunal commented 10 months ago

I just tried this, thanks so much for all the work! Two things I have noticed:

  1. The 'sign in' button on /sign-in displays Sign in twice
  2. The dev server complains about Route ID collisions on startup. I haven't checked yet if this stems from your changes (or if it's an issue we had before..) but it's definitely something to look out for

Let me know if I can help or whenever this is ready!

jeancx commented 10 months ago

I just tried this, thanks so much for all the work! Two things I have noticed:

1. The 'sign in' button on /sign-in displays Sign in twice

2. The dev server complains about Route ID collisions on startup. I haven't checked yet if this stems from your changes (or if it's an issue we had before..) but it's definitely something to look out for

Let me know if I can help or whenever this is ready!

1 - The duplicated Signin text i fixed, just forgot to remove the text after add translation function on side. 2 - That issue had before, it is because exists a file routes/checkout.tsx and also routes/checkout/index.tsx.

jeancx commented 10 months ago

My sugestion would be:

Everything that is inside route folder becomes a website address, so as it uses friendly route, every ´folder/index´ becomes a ´folder´ route, so if there is a root file with same name as a folder with index inside it is a conflict.

jeancx commented 10 months ago

@kyunal it's ready.

kyunal commented 10 months ago

I will review this soon, currently at the Vendure Developer Meetup 👋

kyunal commented 9 months ago

Hi, again thanks so much for all the work! I've been playing around with it for about half an hour, and here's what I found:

jeancx commented 9 months ago
jeancx commented 9 months ago

Solved the hydratation error, it was because the server was not finding the translations files.

jeancx commented 9 months ago

@kyunal You can review again, after the PR is merged i will open another PR and do the language switch.

kyunal commented 9 months ago

Sorry for the incredibly late reply, been busy then sick :(

Looks good to me, and will merge it now! Thanks so much for the work :heart:

michaelbromley commented 9 months ago

Hey @jeancx thank you for this excellent contribution!

I was wondering whether it makes sense to add anything to the readme to first of all make it known that this project has built-in i18n support, but also whether there is anything the dev needs to know to effectively work with it?

michaelbromley commented 9 months ago

@kyunal FYI this commit broke the demo site (https://092412a3.storefront-remix-starter-56q.pages.dev/). I didn't look into why yet, but for now I reverted to the 1eed261 commit which is working fine.

jeancx commented 8 months ago

@kyunal FYI this commit broke the demo site (https://092412a3.storefront-remix-starter-56q.pages.dev/). I didn't look into why yet, but for now I reverted to the 1eed261 commit which is working fine.

Maybe because of the need of node 18.

kyunal commented 8 months ago

There cannot be any node dependencies in most edge runtimes such as Cloudflare Pages which is what's used for the demo site. Specifically, the fs i18next backend required some node deps as well as React's renderToPipeableStream in the entry.server.tsx. I didn't catch this before merging unfortunately, but I addressed this in 554f1f2f350b51a3fe83675e715681869f6c4510 and made your i18n cross-runtime compatible :smile:

jeancx commented 8 months ago

I saw that was fixed 3 days ago, @kyunal if you want i can do the language change button, now that is merged.

kyunal commented 8 months ago

Sure thing! That would be great