feat(i18n): display banner for outdated translations #22

liketechnik · 2024-05-11T18:31:51Z

Displays a "banner" for outdated translations.

'Outdated' is recognized by having an explicit version field in each translation file.

For testing purposes, only the danish translation has a version field, so that the german translation shows as outdated.

I've not worked with tailwind before, so I'm really lost at getting this appropriately styled though.

'Outdated' is recognized by having an explicit version field in each translation file.

liketechnik · 2024-05-11T19:00:47Z

The CI check currently fails due to typescript errors which I am unable to resolve (but the code does work, locally):

npm run astro check

> [email protected] astro
> astro check

20:59:52 Types generated 84ms
20:59:52 [check] Getting diagnostics for Astro files in /home/florian/Documents/programming/contributing/auxolotl/website/feat-translation-outdated-banner...
src/i18n/utils.ts:16:10 - error ts(7053): Element implicitly has an 'any' type because expression of type '"version"' can't be used to index type '{ "root.description": string; "header.community": string; "goals.title": string; "goals.independent.title": string; "goals.independent": string; "goals.gov.title": string; "goals.gov": string; "goals.stabilization.title": string; ... 34 more ...; "values.accessibility": string; } | { ...; } | { ...; }'.
  Property 'version' does not exist on type '{ "root.description": string; "header.community": string; "goals.title": string; "goals.independent.title": string; "goals.independent": string; "goals.gov.title": string; "goals.gov": string; "goals.stabilization.title": string; ... 34 more ...; "values.accessibility": string; } | { ...; } | { ...; }'.

16   return ui[lang]["version"] < ui[defaultLang]["version"];
            ~~~~~~~~~~~~~~~~~~~
src/i18n/utils.ts:10:10 - error ts(7053): Element implicitly has an 'any' type because expression of type '"version" | "i18n-outdated.title" | "i18n-outdated.description" | "root.description" | "header.community" | "goals.title" | "goals.independent.title" | "goals.independent" | ... 37 more ... | "values.accessibility"' can't be used to index type '{ "root.description": string; "header.community": string; "goals.title": string; "goals.independent.title": string; "goals.independent": string; "goals.gov.title": string; "goals.gov": string; "goals.stabilization.title": string; ... 34 more ...; "values.accessibility": string; } | { ...; } | { ...; }'.
  Property 'version' does not exist on type '{ "root.description": string; "header.community": string; "goals.title": string; "goals.independent.title": string; "goals.independent": string; "goals.gov.title": string; "goals.gov": string; "goals.stabilization.title": string; ... 34 more ...; "values.accessibility": string; } | { ...; } | { ...; }'.

10   return ui[lang][key] || ui[defaultLang][key];
            ~~~~~~~~~~~~~

Result (17 files):
- 2 errors
- 0 warnings
- 0 hints

hedyhli

Thanks for this! I've included some possible alternatives with regards to the design of the banner.

src/i18n/utils.ts

hedyhli · 2024-05-13T08:56:51Z

src/components/home/Home.astro

 	<Hero />

+	{
+		i18nIsOutdated(lang) && (
+			<div class="prose prose-invert py-16 px-4 max-w-4xl">
+				<I18nOutdated />
+			</div>
+		)
+	}


How about displaying this as a banner before the Hero instead? The hero does not take up the full width, but the header does. Currently, the warning looks to be a part of the content itself, which isn't.

Suggested change

<Hero />

{

i18nIsOutdated(lang) && (

<div class="prose prose-invert py-16 px-4 max-w-4xl">

<I18nOutdated />

</div>

)

}

{i18nIsOutdated(lang) && <I18nOutdated />}

<Hero />

See suggestion in the i18n component...

So putting it all together:

Keeping it in the same place as before:

<Hero /> { i18nIsOutdated(lang) && ( <div class="pt-16 px-4 w-full max-w-4xl"> <I18nOutdated /> </div> ) } <div class="prose prose-invert pb-16 px-4 max-w-4xl"> <Values /> <Goals /> <Roadmap /> </div>

Component:

class="bg-yellow-800 bg-opacity-50 border-l-4 border-orange-500 text-orange-100 p-4"

A possible design that puts it at the very top, marking the entire page as "possibly outdated":

I've decided to choose your initial suggestion of placing it above the hero. Somehow it attracts my attention much less when at the top. But also, it's much better above the hero than below it.

hedyhli · 2024-05-13T09:04:59Z

src/components/i18n/Outdated.astro

+<section id="i18n-outdated">
+	<h2>{translation("i18n-outdated.title")}</h2>
+	<p class="description">
+		{translation("i18n-outdated.description")}
+	</p>
+</section>


Something like this might look better to serve as a warning, apart from the content on the website itself:

Suggested change

<section id="i18n-outdated">

<h2>{translation("i18n-outdated.title")}</h2>

<p class="description">

{translation("i18n-outdated.description")}

</p>

</section>

<div

class="w-full max-w-4xl bg-yellow-800 bg-opacity-50 border-l-4 border-orange-500 text-orange-100 p-4"

role="alert"

>

<p class="font-bold">{translation("i18n-outdated.title")}</p>

<p>{translation("i18n-outdated.description")}</p>

</div>

Not quite sure about the use of id="i18n-outdated" currently, but feel free to adapt 😄

This stands out much better this way, thanks!

Definitely much better now!

I noticed you still made the entire banner "prose-like". It'd be great to hear what other reviewers think, but I have two suggestions:

I'd recommend against the use of headers in banners, because it competes visually with the hero and header. Although the banner should be attention seeking enough to warn users about the outdated nature of the page, it shouldn't steal attention to become one of the "main titles/headings" of the page. It's a little large right now and breaks the visual hierarchy of the page.

For reference, my suggestion was sourced from: https://v1.tailwindcss.com/components/alerts#left-accent-border

Note that we don't necessarily need prose here -- it's mostly for content-like elements, or ones from the output of say a markdown processor. This is entirely up to you, but for a non-content component like this, elements without prose would work just fine -- extra classes means an extra selectors (and hence possibly redundant properties) applied :)

I noticed you still made the entire banner "prose-like". It'd be great to hear what other reviewers think, but I have two suggestions

I fully agree. Sorry, I was too invested in the original styling of the element, that I didn't realize that you didn't merely move the styling out of Home.astro (I'm working on getting better to thoroughly evaluate alternatives when I'm already invested in a solution - but sometimes I don't; that shouldn't happen, and I try to do better).

Both your suggestions are solid, let me adjust this too...

Ah, no worries!

hedyhli · 2024-05-13T09:23:40Z

Addendum: possible design for a yellow version

bg-yellow-200 bg-opacity-25 border-l-4 border-yellow-500 p-4

Co-authored-by: ~hedy <[email protected]>

liketechnik · 2024-05-13T21:20:06Z

I've decided against the yellow version, because the other one has a higher contrast.

The page looks like this now:

liketechnik · 2024-05-13T21:27:44Z

Hm, the TypeScript error is still not gone 🤔

After some more experimentation:

The error occurs, when the default, i.e. english translation file contains keys that any of the other translation files doesn't. E.g., when adding "just.another.key": "now the typescript check breaks" to public/locales/en/translations.json, the same error occurs. When adding this key to all other translations.json files, it disappears again.

I still have no idea how to solve this, though 🫠

Well, except working around it by adding the new translation keys to all the other language's files.

hedyhli · 2024-05-14T00:24:09Z

Weird, I remember astro check no longer showing errors. But now it does...

When adding this key to all other translations.json files, it disappears again.

Yes, the error arises from ui[lang] not covering all keys of ui[defaultLang], typescript does not let you index "version", since not all possible values for ui[lang] contains "version". As for useTranslation, it does not let you index them the same way since it doesn't know whether key might also be "version" there (or another key that ui[lang] doesn't have).

Explicitly forcing ui[lang] to be seen as ui[defaultLang] would silence the errors (from my testing):

export function useTranslations(lang: keyof typeof ui) {
	return function t(key: keyof (typeof ui)[typeof defaultLang]) {
		return (
			(ui[lang] as (typeof ui)[typeof defaultLang])[key] ||
			ui[defaultLang][key]
		);
	};
}

export function isOutdated(lang: keyof typeof ui) {
	if ("version" in ui[lang]) {
		return (
			(ui[lang] as (typeof ui)[typeof defaultLang])["version"] <
			ui[defaultLang]["version"]
		);
	} else {
		return true;
	}
}

But is this a good idea? I don't have enough experience in TS to answer that. It seems to be working around type safely inappropriately (although perhaps that's just the average TS experience).

mrshmllow · 2024-05-14T03:37:43Z

I was initially concerned with a big banner from reading the code, but after testing I think it looks fine.

mrshmllow · 2024-05-14T03:37:47Z

Addendum: possible design for a yellow version
bg-yellow-200 bg-opacity-25 border-l-4 border-yellow-500 p-4

We should hold off until a real colour scheme is decided on.

mrshmllow · 2024-05-14T03:39:31Z

But is this a good idea? I don't have enough experience in TS to answer that. It seems to be working around type safely inappropriately (although perhaps that's just the average TS experience).

I dont think so. All the documents should contain the same keys instead of forcing types.

mrshmllow · 2024-05-14T03:41:12Z

src/i18n/utils.ts

+	if ("version" in ui[lang]) {
+		return (
+			(ui[lang] as (typeof ui)[typeof lang])["version"] <
+			ui[defaultLang]["version"]
+		);
+	} else {
+		return true;
+	}


All translations should contain the same keys....

Suggested change

if ("version" in ui[lang]) {

return (

(ui[lang] as (typeof ui)[typeof lang])["version"] <

ui[defaultLang]["version"]

);

} else {

return true;

}

return (

(ui[lang] as (typeof ui)[typeof lang])["version"] <

ui[defaultLang]["version"]

);

This confuses me - while I can see where you are coming from, the current useTranslations function contains an ui[lang] || ui[defaultLang] statement, which would be redundant if all translations contain the same keys.

mrshmllow · 2024-05-14T03:54:52Z

Honestly this is not a very well thought out system as a whole. I am concerned with how syncing up the version key will work. What happens when a language gets many little changes and the version goes past english? I dont think weblate will work in this way. These keys should be for UI strings, not prose! I really want to move it away from the website @jakehamilton or have some form of blog system

jakehamilton · 2024-05-14T04:00:43Z

Yeah the solution here isn't ideal. I would've liked to hold off on merging the updated text until the translations were included.

@mrshmllow if you have some ideas feel free to prototype a bit. I think we are quickly approaching the point at which we will need nested elements within translated text, etc.

I suggested Fluent the other day in a call: https://projectfluent.org/

mrshmllow · 2024-05-14T04:10:35Z

nested elements within translated text

Sounds like https://mdxjs.com/

jakehamilton · 2024-05-14T04:19:32Z

nested elements within translated text

Sounds like https://mdxjs.com/

Sorta, but not quite. There are unique differences for some languages where interpolating separate strings does not work and would require different element structure.

hedyhli · 2024-05-14T06:11:50Z

I would've liked to hold off on merging the updated text until the translations were included.

This would ensure all translations are always up-to-date without the need for this PR at all, but this of course isn't feasible, akin to software maintainers having to alert packagers on updated versions, and packages not always being up-to-date.

Honestly this is not a very well thought out system as a whole. I am concerned with how syncing up the version key will work. What happens when a language gets many little changes and the version goes past english?

It's far from perfect, but this PR only adds a solution for identifying outdated translations on top of the current JSON-based system. As I understand it, the version would be incremented each time the English locale is updated that would necessarily require updates from other locales. This makes the version key of other locales fall behind and hence the banner is shown.

I'm not sure how a language other than English could have changes that can be more up-to-date the the English version, though?

We should hold off until a real colour scheme is decided on.
[...]
I really want to move it away from the website @jakehamilton or have some form of blog system.

Agreed, the sooner the better. We currently have a few open translation PRs and it's a burden having to keep resolving new changes and syncing from upstream under the current system. I've started a topic on Discourse regarding this and it'd be great to decide on a better translation system soon.

However, our current two locales in production remains to be outdated. A color scheme is also yet to be decided. Personally I think it's best to have at least a warning for the dk and de pages right now and update the colors later, once we have a better system.

Co-authored-by: ~hedy <[email protected]>

liketechnik force-pushed the feat/translation-outdated-banner branch from ebec513 to 8e0ec28 Compare May 11, 2024 18:34

feat(i18n): display banner for outdated translations

fa47a92

'Outdated' is recognized by having an explicit version field in each translation file.

liketechnik force-pushed the feat/translation-outdated-banner branch from 8e0ec28 to fa47a92 Compare May 11, 2024 18:59

hedyhli suggested changes May 13, 2024

View reviewed changes

hedyhli added enhancement New feature or request labels May 13, 2024

liketechnik and others added 2 commits May 13, 2024 22:54

fix(i18n/utils): typechecking errors

2a3b84d

Co-authored-by: ~hedy <[email protected]>

fix(i18n): make outdated banner stand out

0f193b0

Co-authored-by: ~hedy <[email protected]>

mrshmllow reviewed May 14, 2024

View reviewed changes

fix(i18n): stop using banner for outdated notification banner

51bb222

Co-authored-by: ~hedy <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(i18n): display banner for outdated translations #22

feat(i18n): display banner for outdated translations #22

liketechnik commented May 11, 2024

liketechnik commented May 11, 2024

hedyhli left a comment

hedyhli May 13, 2024 •

edited

Loading

liketechnik May 13, 2024

hedyhli May 13, 2024

liketechnik May 13, 2024 •

edited

Loading

hedyhli May 14, 2024

liketechnik May 14, 2024

hedyhli May 14, 2024

hedyhli commented May 13, 2024

liketechnik commented May 13, 2024

liketechnik commented May 13, 2024

hedyhli commented May 14, 2024

mrshmllow commented May 14, 2024

mrshmllow commented May 14, 2024

mrshmllow commented May 14, 2024

mrshmllow May 14, 2024

liketechnik May 14, 2024

mrshmllow commented May 14, 2024 •

edited

Loading

jakehamilton commented May 14, 2024

mrshmllow commented May 14, 2024

jakehamilton commented May 14, 2024

hedyhli commented May 14, 2024 •

edited

Loading

feat(i18n): display banner for outdated translations #22

Are you sure you want to change the base?

feat(i18n): display banner for outdated translations #22

Conversation

liketechnik commented May 11, 2024

liketechnik commented May 11, 2024

hedyhli left a comment

Choose a reason for hiding this comment

hedyhli May 13, 2024 • edited Loading

Choose a reason for hiding this comment

liketechnik May 13, 2024

Choose a reason for hiding this comment

hedyhli May 13, 2024

Choose a reason for hiding this comment

liketechnik May 13, 2024 • edited Loading

Choose a reason for hiding this comment

hedyhli May 14, 2024

Choose a reason for hiding this comment

liketechnik May 14, 2024

Choose a reason for hiding this comment

hedyhli May 14, 2024

Choose a reason for hiding this comment

hedyhli commented May 13, 2024

liketechnik commented May 13, 2024

liketechnik commented May 13, 2024

hedyhli commented May 14, 2024

mrshmllow commented May 14, 2024

mrshmllow commented May 14, 2024

mrshmllow commented May 14, 2024

mrshmllow May 14, 2024

Choose a reason for hiding this comment

liketechnik May 14, 2024

Choose a reason for hiding this comment

mrshmllow commented May 14, 2024 • edited Loading

jakehamilton commented May 14, 2024

mrshmllow commented May 14, 2024

jakehamilton commented May 14, 2024

hedyhli commented May 14, 2024 • edited Loading

hedyhli May 13, 2024 •

edited

Loading

liketechnik May 13, 2024 •

edited

Loading

mrshmllow commented May 14, 2024 •

edited

Loading

hedyhli commented May 14, 2024 •

edited

Loading