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

chore(i18n): add a 'count=1' overwritable global variable #3318

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

johncowen
Copy link
Contributor

@johncowen johncowen commented Dec 18, 2024

It would be nice to use pluralization via our i18n more (instead of hardcoding both singular and plural forms as separate keys)

I've kinda experimented with this before, so this more of sharing that thinking and therefore this PR might not eventually be merged.


This PR adds a default count "global" variable.

We already have an idea of global variables but they are almost like environment variables and cased so.

Why do we need a count global variable?

I've included a name example in this PR. Almost every resource in our application has a http name property and we use that term everywhere in our UI. Sometimes we want names and sometimes we want name.

Instead of having two keys:

name: Name
names:  |
      { count, plural,
          =0 { Names }
          =1 { Name }
          other { Names }
        }

or worse

name: Name
names: Names

we could just have one:

name:  |
      { count, plural,
          =0 { Names }
          =1 { Name }
          other { Names }
        }

The thing is that would mean you always have to pass a count which is annoying.

I tried some other ideas first (such as checking the error returned when count was missing) but seemed too over-complicated.

Copy link

netlify bot commented Dec 18, 2024

Deploy Preview for kuma-gui ready!

Name Link
🔨 Latest commit fb4d92e
🔍 Latest deploy log https://app.netlify.com/sites/kuma-gui/deploys/6762b26ac67fef0009f8be5f
😎 Deploy Preview https://deploy-preview-3318--kuma-gui.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.

@johncowen
Copy link
Contributor Author

Taking this out of draft, but only so it opens up review. Still kinda in a "not even sure I want to merge this" state

@johncowen johncowen marked this pull request as ready for review December 19, 2024 10:32
@johncowen johncowen requested a review from a team as a code owner December 19, 2024 10:32
@johncowen johncowen requested review from schogges and removed request for a team December 19, 2024 10:32
@schogges
Copy link
Contributor

schogges commented Jan 6, 2025

I also have mixed feelings 🤔 to me it feels more natural if there is no global count, but in cases I always want plural I'd not pass count (which leads to undefined -> other, please correct me if I am wrong here).
What cases do we actually have where we need to pass count with hardcoded value? I can think of table headers where we don't want to have plural and pass count: 1 🤔

@johncowen
Copy link
Contributor Author

(which leads to undefined -> other, please correct me if I am wrong here)

From what I remember I also thought undefined would lead to other but as you specify count in the i18n string you have to give it a count even if its count: undefined (I think this is what I found out, but I did this before the break)

What cases do we actually have where we need to pass count with hardcoded value? I can think of table headers where we don't want to have plural and pass count: 1 🤔

I suppose anywhere where we refer to a key/prop. The idea would be that we would do this for a multiple props, not just name. I guess we would add these to our http.api.property set of strings 🤔 ? Meaning that all our http.api.property would be singular by default, but you could make them plural by adding count: items.length (or just count: 2). We also use them for resource names, which is where I think it would be most useful i.e. Mesh/Meshes, Dataplane/Dataplanes, Zone/Zones etc etc.

@schogges
Copy link
Contributor

schogges commented Jan 7, 2025

(which leads to undefined -> other, please correct me if I am wrong here)

From what I remember I also thought undefined would lead to other but as you specify count in the i18n string you have to give it a count even if its count: undefined (I think this is what I found out, but I did this before the break)

Oh, my assumption might be wrong about this 😅 will double check again

What cases do we actually have where we need to pass count with hardcoded value? I can think of table headers where we don't want to have plural and pass count: 1 🤔

I suppose anywhere where we refer to a key/prop. The idea would be that we would do this for a multiple props, not just name. I guess we would add these to our http.api.property set of strings 🤔 ? Meaning that all our http.api.property would be singular by default, but you could make them plural by adding count: items.length (or just count: 2). We also use them for resource names, which is where I think it would be most useful i.e. Mesh/Meshes, Dataplane/Dataplanes, Zone/Zones etc etc.

Hmmm 🤔 but in the case of http.api.property we don't need to add anything to the yaml files, right? At least for the keys in configs it's more like that we want to show them without any transformation?

@johncowen
Copy link
Contributor Author

Hmmm 🤔 but in the case of http.api.property we don't need to add anything to the yaml files, right? At least for the keys in configs it's more like that we want to show them without any transformation?

We don't need to add anything, but we should do, ideally we would completely delete the camelCase thing.

I think in the case of resources names it would be particularly handy, I'm not totally sure they would go in http.api.property but maybe just a the root of a modules i18n file:

meshes:
  resourceName: { count, plural,
          =0 { Meshes }
          =1 { Mesh }
          other { Meshes }
        }
t('meshes.resourceName') // Mesh because of global `count`
t('meshes.resourceName', { count: items.length }) // Meshes
...
t('etc.resourceName') // Thing because of global `count`
t('etc.resourceName', { count: items.length }) // Things

@schogges
Copy link
Contributor

schogges commented Jan 8, 2025

Hmmm 🤔 but in the case of http.api.property we don't need to add anything to the yaml files, right? At least for the keys in configs it's more like that we want to show them without any transformation?

We don't need to add anything, but we should do, ideally we would completely delete the camelCase thing.

Ok, I like that and agree here 👍

I think in the case of resources names it would be particularly handy, I'm not totally sure they would go in http.api.property but maybe just a the root of a modules i18n file:

meshes:
  resourceName: { count, plural,
          =0 { Meshes }
          =1 { Mesh }
          other { Meshes }
        }
t('meshes.resourceName') // Mesh because of global `count`
t('meshes.resourceName', { count: items.length }) // Meshes
...
t('etc.resourceName') // Thing because of global `count`
t('etc.resourceName', { count: items.length }) // Things

I wasn't sure sure if we really need both singular and plural for resource names, but at least for page titles and breadcrumbs we might need it. But setting global count to 1, for titles that would mean

t('meshes.resourceName', { count: 2 }) // Meshes

Looks a little odd to me 🤔

Or would you treat page titles differently? Like

t('meshes.routes.items.title') // Meshes

@johncowen
Copy link
Contributor Author

t('meshes.resourceName', { count: 2 }) // Meshes
Looks a little odd to me 🤔

Hmm yeah to me too 🤔 , not sure if this pushes it too far but maybe we make:

`t('meshes.resourceName', {}, { plural: true })

(note plural is an option not a param)

turn into t('meshes.resourceName', { count: 2 }), behind the scenes.

Maybe a bit too much magic there though, still thinking myself, but lemme know if you had any thoughts on that one ^

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