welpo / tabi

A modern Zola theme with search, multilingual support, optional JavaScript, a perfect Lighthouse score, and a focus on accessibility.
https://welpo.github.io/tabi/
MIT License
97 stars 32 forks source link

✨ feat(i18n): implement pluralization logic #277

Closed TheAwiteb closed 4 months ago

TheAwiteb commented 4 months ago

Ref https://github.com/welpo/tabi/pull/276

Proof of concept pluralization for testing.

Discussion: #273

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

To test:

ss

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

TheAwiteb commented 4 months ago

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

welpo commented 4 months ago

My bad!

welpo commented 4 months ago

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

TheAwiteb commented 4 months ago

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?

welpo commented 4 months ago
{{ 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.

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

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

TheAwiteb commented 4 months ago

Great, I'll start on it tomorrow

welpo commented 4 months ago

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 commented 4 months ago

Identify strings that need pluralisation:

As I see is

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

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

TheAwiteb commented 4 months ago

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

welpo commented 4 months ago

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 commented 4 months ago

Same with zero_min_read. I tried to trigger it with a single letter post, but it looks like 1 minute is the minimum.

EDIT: Nevermind, it can be triggered with a zero-words post…

TheAwiteb commented 4 months ago

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

TheAwiteb commented 4 months ago

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 commented 4 months ago

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:

welpo commented 4 months ago

Almost done! Search now uses the new strings…

https://github.com/welpo/tabi/assets/6399341/5e2b7e27-3539-43cc-8918-197f73e5b059

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 commented 4 months ago

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:

TheAwiteb commented 4 months ago

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

I'll, in the morning

TheAwiteb commented 4 months ago

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

TheAwiteb commented 4 months ago
  • Ensure proper fallback to English if string is missing

I'll check soon as I get to my laptop

TheAwiteb commented 4 months ago
  • Ensure proper fallback to English if string is missing

All is good as expected

welpo commented 4 months ago

Thank you! I will do some testing later.

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

welpo commented 4 months ago

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~