-
Notifications
You must be signed in to change notification settings - Fork 2
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
Spanish website translation + Hability to localize the site #2
Conversation
Signed-off-by: delthia <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for your work, really appreciate it!
I've now extensively read through your changes and reviewed them. They are very good and it already looks very nice, kudos to your work! I have just added some comments for fine-tuning and code quality/consistency, and one comment for a small logic issue.
A tip for the formatting: This project includes an .editorconfig
, which configures your IDE to use the correct format for text files (at least at the base level). I don't know which IDE you use but many editors support this file, natively or via extension, and it helps a lot keeping the format automatically clean.
config/_default/config.toml
Outdated
assetDir = "src" | ||
enableRobotsTXT = true | ||
|
||
title = "FlorisBoard" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing empty line at the end (this is a convention I use throughout all my projects, because it allows to print a file in the terminal quickly without messed up formats).
footer = "For more information and download options check out the GitHub repo!" | ||
modificated = "Last modified:" | ||
florisboard-icon = "FlorisBoard app icon" | ||
florisboard-screenshot = "FlorisBoard screenshot" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, missing empty line at the end
footer = "¡Para más información y opciones de descarga echa un vistazo al repositorio de GitHub!" | ||
modificated = "Modificado por última vez el" | ||
florisboard-icon = "Icono de la aplicación FlorisBoard" | ||
florisboard-screenshot = "Captura de pantalla de FlorisBoard" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, missing empty line at the end
config/_default/config.toml
Outdated
@@ -0,0 +1,8 @@ | |||
# baseURL = "https://florisboard.org/" | |||
baseURL = "https://f.delthia.com/" | |||
defaultContentLanguage = 'en' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest using a double quote for the en-string, to be in line with the other strings in the config.
config/_default/config.toml
Outdated
@@ -0,0 +1,8 @@ | |||
# baseURL = "https://florisboard.org/" | |||
baseURL = "https://f.delthia.com/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For testing this is OK, but we must not forget changing this back to the florisboard.org domain before merging in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this could be made to work by having a different action when it is in a fork or on another branch, I haven't much experience with that, but if I find a simple way to make it work like that, maybe it would be easier if people contribute translations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I don't really know if an action can be configured to run with different params tbh, and even then how do you configure this param without having to commit it to the fork?
layouts/404.html
Outdated
@@ -1,3 +1,3 @@ | |||
{{ define "main" }} | |||
<p style="color: red;">The requested resource could not be found. Please check the address and try again.</p> | |||
<p style="color: red;">{{ .Param "not-found" }}</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure, but can't one use {{ i18n "string_name" }}
for referencing strings in Hugo? If so I think using this would be best, as it is immediately clear that this variable is a translatable variable.
(if it is possible to use the i18n syntax this comment is valid for all usage sites, but I won't comment individually on all occurrences)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it that way because it was the quickest I found, as I haven't much experience with the multilang part of hugo, but now that the translations where splitted into diferent files, I think it would be feasible to use the i18n syntax after I look into it.
layouts/_default/single.html
Outdated
@@ -1,5 +1,5 @@ | |||
{{ define "main" }} | |||
<h1>{{ .Title }}</h1> | |||
{{ with .Lastmod }}<p class="last-modified">Last modified: {{ . | time.Format ":date_medium" }}</p>{{ end }} | |||
<p class="last-modified">{{ .Param "modificated" }} {{ with .Lastmod }}{{ . | time.Format ":date_medium" }}</p>{{ end }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you moved the {{ with .Lastmod }}
block inwards, causing half of the last mod paragraph to always be included. It should be before the paragraph element, so it either includes the whole localized paragraph or not at all if the lastmod attribute is not defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll have to look a bit more into this, as I changed it because otherwise the translation variable would work. It does make sense like that, but as currently the site only has a single page that does contain the lastmod tag, I didn't think about that.
Thanks for that, I just read through them, but tomorrow I'll go through them in more depth to fix them,
Well, when it comes to that, I am a bit all over the place, I used to use atom, and I've recently changed to vscodium, but I also use vim, in reality I don't get along to well with IDEs, and I end up just using them as code editors and have a few terminals, as I mostly work with small python and bash scripts. Thanks for the tip! I should look into that, because atom did add the extra line at the end. |
Hi, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey delthia,
thanks a lot for addressing the mentioned issues!
I've reviewed the changes and the i18n management is now really cool and machine-accessible, so it can at some point easily be integrated into a translation service. I agree with you that putting language-specific configs in their own file is a good option.
So the only issue I now see is that the language switcher can look janky on mobile pages on small screens, however I think this can be fixed in the future by another PR, especially as the "Visit GH" link needs a rework too at some point.
So if you don't have anything to add I think this PR is merge-ready, great work :) Only thing we must not forget is to change the base URL back to florisboard.org before merging.
Hi, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the fixes!
I now tested your changes locally and everything works like a charm. Only thing I found was that the link to the wiki page is broken within the Spanish version of the privacy policy (see comment). Once you accept the suggested change I will merge your changes in.
I think I will use Crowdin for the website as well. It would make sense at least, as the main project is also using Crowdin. But atm my head is too full that I can focus on this integration, as I need to focus on my word suggestions project, which is moving a lot slower than I want sadly. |
Co-authored-by: Patrick Goldinger <[email protected]>
I've just accepted the changes, so everything is ready.
I hope it goes well with that project, as I really like the keyboard and that is a great feature. Thanks again for the project, I'll keep an eye around for when I can help with the page, as it the area I can help in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes, will now merge in your changes!
As talked in #1, this adds the spanish translation of the site, and all the needed configuration for the site to be more easily translated, including a simple language switcher.