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.

Sign up for GitHub

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

Jump to bottom

Use next-translate for i18n #192

Merged
merged 1 commit into from
Oct 10, 2021
Merged

Use next-translate for i18n #192

merged 1 commit into from
Oct 10, 2021

Conversation

typeofweb
Copy link
Contributor

@typeofweb typeofweb commented Oct 6, 2021 •
edited
Loading

Fixes #108

  • Installed & configured next-translate
  • Made it possible to specify desired language via data-lang attribute
  • Added English translation
  • Added Polish translation
  • Make it possible to translate formatDateDistance

@vercel
Copy link

vercel bot commented Oct 6, 2021

@mmiszy is attempting to deploy a commit to the giscus Team on Vercel.

A member of the Team first needs to authorize it.

@typeofweb typeofweb mentioned this pull request Oct 6, 2021
Closed
@vercel
Copy link

vercel bot commented Oct 7, 2021 •
edited
Loading

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/giscus/giscus/6UL1WFagdcNGn411w3e5ghtFiVUD
✅ Preview: https://giscus-git-fork-typeofweb-i18n-giscus.vercel.app

laymonage
laymonage reviewed Oct 7, 2021
View reviewed changes
Copy link
Member

@laymonage laymonage left a comment

Choose a reason for hiding this comment

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

Fantastic work, thank you very much! I left some comments. I've tried fixing some of them locally but I can't seem to push the changes to your fork.

pages/widget.tsx Outdated Show resolved Hide resolved
locales/en/common.json Outdated Show resolved Hide resolved
locales/en/common.json Outdated Show resolved Hide resolved
locales/en/common.json Outdated Show resolved Hide resolved
locales/pl/common.json Outdated Show resolved Hide resolved
lib/i18n.ts Outdated Show resolved Hide resolved
next.config.js Outdated Show resolved Hide resolved
@typeofweb
Copy link
Contributor Author

I can't seem to push the changes to your fork.
I've accidentally forked this repo from my organisational account and that's why. Sorry about that.

I'll look into the setLanguage function and why it's behaving weirdly

@typeofweb typeofweb marked this pull request as draft October 8, 2021 11:47
@typeofweb typeofweb marked this pull request as ready for review October 9, 2021 22:54
@typeofweb
Copy link
Contributor Author

Updated @laymonage
Now translations are loaded semi-manually just for the widget.

laymonage
laymonage reviewed Oct 10, 2021
View reviewed changes
Copy link
Member

@laymonage laymonage left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hey @mmiszy, thanks for the update!

Unfortunately it seems to use internal APIs of next-translate, which is not very ideal.

I managed to fix the useEffect issue by moving it to the Widget component (in components) instead of the widget page (in pages). You can see it in this commit. You can see the i18n branch on this repository for more details.

However, I noticed that your approach loads the translation server-side, so there's no flash of English language on the initial render, which is great. But, it's achieved through more code & the use of internal APIs. I'm not sure which one is better... what's your opinion on this? 😄

locales/pl/common.json Outdated Show resolved Hide resolved
@typeofweb
Copy link
Contributor Author

Looking at the source of setLanguage we can see all it does is a redirect.

That's why I've decided to do a bit of more manual work, load proper locales on the server and avoid changing the URL of the widget.

laymonage
laymonage reviewed Oct 10, 2021
View reviewed changes
Copy link
Member

@laymonage laymonage left a comment

Choose a reason for hiding this comment

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

Hey, there's only minor issues left. Normally I would just fix these for you, squash the commits, and merge the PR right now, but I cannot because I don't have the permission. Maybe you left the "Allow edits by maintainers" unchecked or maybe because it's from your organization's fork.

Could you please check whether you could enable the following?

https://docs.github.com/en/github/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

If the option is available and not enabled, please enable it and let me know, and ignore the rest of this comment.

Otherwise, could you:

git pull upstream main
git rebase upstream/main -i

And squash the commits into a single commit, and then

git push origin i18n -f

(I need this so that the diff view on GitHub is up to date and doesn't contain my existing changes for the Supabase config.)

If it's too much hassle, then don't worry, I will just use GitHub's squash merge option.

components/Comment.tsx Outdated Show resolved Hide resolved
client.ts Outdated Show resolved Hide resolved
@typeofweb typeofweb force-pushed the i18n branch 2 times, most recently from a6d423a to 4307322 Compare October 10, 2021 10:25
@typeofweb
Copy link
Contributor Author

@laymonage done with the rebase. I can't enable edits by maintainers because it's my organisation's fork. In the future I'll create PRs from my private account :D

laymonage
laymonage reviewed Oct 10, 2021
View reviewed changes
Copy link
Member

@laymonage laymonage left a comment

Choose a reason for hiding this comment

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

Whoops, sorry, looks like it failed to build. Could you fix this?

image

To make sure no other issues are present, maybe you can try building the project locally before pushing it to GitHub.

@typeofweb @laymonage
Use next-translate for i18n
31a9a5b 
Add English and Polish translations.

Add missing date formatter for Polish

Add missing formatDate and use Prettier

Cleanup

Update lib/i18n.ts

Co-authored-by: sag᠎e <laymonage@gmail.com>

Fix setting language

Update lib according to CR

Fix other -> many

Update client.ts

Co-authored-by: sag᠎e <laymonage@gmail.com>

Update components/Comment.tsx

Co-authored-by: sag᠎e <laymonage@gmail.com>
@laymonage laymonage merged commit 7828b61 into giscus:main Oct 10, 2021
@laymonage
Copy link
Member

Thank you very much @mmiszy! I will add proper support for i18n (include it in the configuration step, update the contributing guide, etc.) in a future PR.

Welcome aboard! 🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@laymonage laymonage laymonage approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Localization support
2 participants
@typeofweb @laymonage