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

✨ feat(i18n): implement pluralization logic #277

Merged
merged 23 commits into from
Feb 16, 2024

Conversation

TheAwiteb
Copy link
Collaborator

@TheAwiteb TheAwiteb commented Feb 12, 2024

Ref #276

Proof of concept pluralization for testing.

Discussion: #273

The logic to get the strings is in the macro pluralize.html.

To test:

  • Set language to arabic (already done)
  • Modify templates/page.html to change the count variable in the Debugging section
  • Load any post to see a table with the values and translations:

ss

🚨 IMPORTANT: when you change the TOML strings in i18n, you need to restart zola serve to use the updated strings.

@TheAwiteb
Copy link
Collaborator Author

Yes, this b6aa219 took 51 minutes of debugging. it was always says the language is en

@welpo
Copy link
Owner

welpo commented Feb 12, 2024

My bad!

@welpo
Copy link
Owner

welpo commented Feb 12, 2024

Let me know if you want any help or have any questions, by the way! Happy to help here.

@TheAwiteb
Copy link
Collaborator Author

TheAwiteb commented Feb 12, 2024

Let me know if you want any help or have any questions, by the way! Happy to help here.

Now the macro works good with Arabic, what next?

@TheAwiteb TheAwiteb force-pushed the feat/proper-pluralization branch from 5a09ba3 to 62fc7de Compare February 12, 2024 21:23
@welpo
Copy link
Owner

welpo commented Feb 12, 2024

  • Figure out a way to simplify the macro calls; I don't want 5 lines of Tera every time we need a translation with plurals. Perhaps we can call a macro from within a macro? Ideally we would change a line like:
{{ macros_translate::translate(key='result', default='result', language_strings=language_strings) }}

to:

{{ macros_translate::translate(key='result', default='result', number=X, language_strings=language_strings) }}

Or something like this. Not sure how feasible this is, though. Just an idea.

  • Identify strings that need pluralisation:
    • search results
    • minutes to read
    • post/posts? Not sure about this
    • number of words (words)
    • I might be missing something, not sure

All of these are HTML strings, except for the search results. I'll take care of modifying the JavaScript at the end.

  • Add/modify the variable names for these. At least agree on the prefixes first
  • Expand pluralization macro for other languages (best effort). I can take care of this

Once all of this is done, modify existing i18n files to use the new variables names. Same with the macro calls.

@TheAwiteb
Copy link
Collaborator Author

Great, I'll start on it tomorrow

@welpo
Copy link
Owner

welpo commented Feb 13, 2024

Changes

Integrate pluralize.html into translate.html

Since translate was simple enough, I've integrated pluralization into it. This simplifies getting the pluralised keys. Now we can do:

{%- macros_translate::translate(key=base_key, number=count, language_strings=language_strings, default="couldn't find the string") -%}

Where number is optional. If it's there, pluralisation logic kicks in. Else, it behaves as before, directly translating key.

Convert $NUMBER

If a number is provided to the new macro, the string $NUMBER will get replaced for the number provided. This makes it more general, instead of needing to handle specific $VARIABLEs.

@TheAwiteb
Copy link
Collaborator Author

TheAwiteb commented Feb 13, 2024

Identify strings that need pluralisation:

As I see is

  • result
  • post
  • min_read
  • words

At least agree on the prefixes first

I see zero_, one_, tow_, few_ and many_ is good

Note

This only for Arabic (for now) other languges will only have zero_, one_ and many_

I'll work on it now

@welpo
Copy link
Owner

welpo commented Feb 13, 2024

This only for Arabic (for now) other languges will only have zero_, one_ and many_

Most languages use many for the 0 case, so we'll only need many and one.

@TheAwiteb
Copy link
Collaborator Author

Most languages use many for the 0 case, so we'll only need many and one.

I don't see this is good, because for example in result if there is no search result we can't say "0 results" I think "No result for your query" or something like this is better. What do you think?

@TheAwiteb
Copy link
Collaborator Author

for example in result if there is no search result we can't say "0 results" I think "No result for your query" or something like this is better. What do you think?

Also min_read for zero_ I think "less than minute" is better than "0 minutes"

@TheAwiteb
Copy link
Collaborator Author

If you don't think so, I will add it in Arabic only, because in Arabic we can't say "0 minutes" or "0 results" it is against the laws of language

@welpo
Copy link
Owner

welpo commented Feb 13, 2024

if there is no search result we can't say "0 results" I think "No result for your query" or something like this is better

It does sound better. My main concern is keeping the i18n files small, so that they are easy to review and adapt.

Also min_read for zero_ I think "less than minute" is better than "0 minutes"

Agreed.

Alright, let's go with zero, one and many for all number-related strings.


It makes me sad to realise we'll probably have to duplicate logic for the pluralisation, though:

    function updateResultText(count) {
        const nResultsSpan = document.getElementById('n-results');
        nResultsSpan.textContent = count.toString();

        const singular = document.getElementById('result-text-singular');
        const plural = document.getElementById('result-text-plural');

        if (count === 1) {
            singular.style.display = 'inline';
            plural.style.display = 'none';
        } else {
            singular.style.display = 'none';
            plural.style.display = 'inline';
        }
    }

This is the current search JS. Oh, well. It's better to duplicate logic than to load JS just to do pluralisation, imo.

@TheAwiteb
Copy link
Collaborator Author

TheAwiteb commented Feb 13, 2024

This is the current search JS. Oh, well. It's better to duplicate logic than to load JS just to do pluralisation, imo.

I will work on translations now, and keep JS things for you

@welpo
Copy link
Owner

welpo commented Feb 13, 2024

I will work on translations now, and keep JS things for you

You can do English and Arabic. I'll do the rest!

@TheAwiteb
Copy link
Collaborator Author

@welpo Now should the remaining is JS and translating languages else than Arabic and English

@welpo
Copy link
Owner

welpo commented Feb 13, 2024

I think we can remove the zero_posts case, no? If we're showing a tag —that's where the key is used— it's because there's at least one post to show.

@welpo welpo force-pushed the feat/proper-pluralization branch from ee5d269 to 3791ffc Compare February 13, 2024 23:37
@TheAwiteb
Copy link
Collaborator Author

Are we done like this? Do I prepare it for review? (Remove debugging and so on)

@TheAwiteb
Copy link
Collaborator Author

TheAwiteb commented Feb 14, 2024

btw, I really like the way you merge the PR since I don't really have to make the PR/branch log very clean (because it won't appear in the master, it just shows commit as the PR)

@welpo
Copy link
Owner

welpo commented Feb 14, 2024

btw, I really like the way you merge the PR since I don't really have to make the PR/branch log very clean (because it won't appear in the master, it just shows commit as the PR)

Yeah, squash-and-merge pull requests are pretty cool! I like how you get to keep the atomic commits for comparison yet keep a clean main history.

Are we done like this? Do I prepare it for review? (Remove debugging and so on)

Nope! I need to add:

  • add the new strings for Ukranian and Russian
  • implement the JS changes
  • do a lot of testing to ensure (1) we're not missing anything and (2) nothing breaks

@welpo
Copy link
Owner

welpo commented Feb 14, 2024

Almost done! Search now uses the new strings…

s.mov

Can't test with Arabic, as Zola can't build an index for it. However, I replicated the logic for pluralization anyway; if it ever gets added, it should work without any changes.

@welpo
Copy link
Owner

welpo commented Feb 15, 2024

Added a new replace parameter to translate. When set to true (default), replaces $NUMBER with the number parameter.

I needed to do this so that JavaScript could dynamically update the $NUMBER.


I think this is done! Tomorrow I will do a careful review of all changes + some more testing.

If you can do some testing to ensure everything works well, I'd appreciate it!

Off the top of my head:

  • Ensure proper fallback to English if string is missing
  • Consistent prefix use
  • Consistent $NUMBER use
  • All new strings available on all languages

@TheAwiteb
Copy link
Collaborator Author

If you can do some testing to ensure everything works well, I'd appreciate it!

I'll, in the morning

@TheAwiteb
Copy link
Collaborator Author

Sorry for the delay, but I've been busy with university. I checked the following and they are all ok

  • All new translations have been added to all languages.
  • All new translation keys contain no spelling errors.
  • All of them who need $NUMBER have it.
  • New translations are used correctly with the translate macro.

@TheAwiteb
Copy link
Collaborator Author

  • Ensure proper fallback to English if string is missing

I'll check soon as I get to my laptop

@TheAwiteb
Copy link
Collaborator Author

  • Ensure proper fallback to English if string is missing

All is good as expected

@welpo
Copy link
Owner

welpo commented Feb 16, 2024

Thank you! I will do some testing later.

If everything is good, I will remove the debugging stuff and merge.

@welpo welpo changed the title 🚧 feat: proof of concept pluralization ✨ feat(i18n): implement pluralization logic Feb 16, 2024
@welpo welpo marked this pull request as ready for review February 16, 2024 14:38
@welpo welpo self-requested a review as a code owner February 16, 2024 14:38
@welpo
Copy link
Owner

welpo commented Feb 16, 2024

I think it's done!

I'll merge now. Thank you so much for suggesting this feature and working to make it a reality!

It's been a pleasure collaborating~

@welpo welpo merged commit c4893d4 into welpo:main Feb 16, 2024
4 of 5 checks passed
Smtbook pushed a commit to Smtbook/zola-theme-tabi that referenced this pull request Feb 29, 2024
Co-authored-by: welpo <welpo@users.noreply.github.com>
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