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

Fix: giscus.html fails when country code is specified. #358

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dogweather
Copy link
Contributor

@dogweather dogweather commented Apr 15, 2024

The problem is that the Giscus API fails when given data-lang of something like de-DE:

Screenshot 2024-04-15 at 15 40 00

This fix uses just the language tag, not the whole language code: https://gohugo.io/methods/site/language/#methods

With this fix:

Screenshot 2024-04-15 at 15 47 13

(live at https://forkful.ai/de/kotlin/good-coding-practices/ )

Copy link

netlify bot commented Apr 15, 2024

Deploy Preview for hugo-hextra ready!

Name Link
🔨 Latest commit 27a70e4
🔍 Latest deploy log https://app.netlify.com/sites/hugo-hextra/deploys/661dcc21350f64000885cbcf
😎 Deploy Preview https://deploy-preview-358--hugo-hextra.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.

@dogweather dogweather closed this Apr 15, 2024
@dogweather dogweather deleted the patch-4 branch April 15, 2024 21:51
@dogweather dogweather restored the patch-4 branch April 15, 2024 21:51
@dogweather dogweather reopened this Apr 15, 2024
@imfing
Copy link
Owner

imfing commented Apr 15, 2024

Thanks for the PR. It makes sense.

I checked the list of available locales on giscus and noticed that zh-CN and zh-TW use mixed casing.
With this change, the giscus will now use the lowercase zh-cn in our example site, which can be seen here:

Could you try changing zh-cn to zh-CN in the above link to see if everything still works correctly?

@dogweather
Copy link
Contributor Author

Huh, will do. I'm guessing we need to make a special case for those. And yep, currently on my site, using just zh, it's broken.

@dogweather
Copy link
Contributor Author

With this change, the giscus will now use the lowercase zh-cn in our example site, which can be seen here:

Could you try changing zh-cn to zh-CN in the above link to see if everything still works correctly?

I'm confused, sorry. With my change, it will just use the first two letters of the code. Here's my debug output I verified this with:

".Language.Lang": "zh"
".Language.LanguageCode": "zh-CN"

And that currently breaks because giscus doesn't have i18n support for simple zh.

@dogweather
Copy link
Contributor Author

dogweather commented Apr 16, 2024

I added special handling for Chinese. I tested it with:

languageCode: zh
languageCode: zh-cn
languageCode: zh-CN
languageCode: zh-tw
languageCode: zh-TW

@imfing
Copy link
Owner

imfing commented Apr 16, 2024

Awesome,appreciate it 👍
I was asking because of this issue #231

Will review it later today

Giscus uses the geophraphical language code for these.
See: https://github.com/giscus/giscus/tree/main/locales
*/}}
{{ if eq $giscus_lang "zh" }}
Copy link
Owner

Choose a reason for hiding this comment

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

In this case, $giscus_lang could also be zh-cn, so it's better to use hasPrefix instead of eq.

@@ -1,4 +1,26 @@
{{- $lang := site.Language.LanguageCode | default `en` -}}
{{ $default_chinese := "zh-CN" }}
Copy link
Owner

Choose a reason for hiding this comment

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

Should we just inline this $default_chinese, or move it inside the if block?

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