Added translation banner - #602
Conversation
|
Just a quick disclaimer before this: I'm away for the weekend and not at my computer, so I've only given the PR a relatively quick look from my phone. I also passed the implementation through ChatGPT to help spot anything I might have missed. So please treat this as an initial review rather than a thorough code review. 😄 That said, I think this is honestly a very elegant and clever solution. Centralizing the notice in a component is much better than adding a permanent From my quick look, the overall approach seems correct, but I noticed one potentially important regression and a few edge cases that might be worth checking before merging. The main thing is that overriding Starlight's The custom component should probably import and render the default banner as well: ---
import DefaultBanner from '@astrojs/starlight/components/Banner.astro';
---
<DefaultBanner><slot /></DefaultBanner>The translation notice can then be rendered alongside it. I would also add The other thing I would consider changing is the AI detection. The current implementation eagerly imports the raw contents of all translated files and searches the frontmatter for the string It might be cleaner to extend the existing docsSchema({
extend: z.object({
ai_translated: z.boolean().optional().default(false),
}),
})The component could then directly read something along the lines of: const { entry, locale, isFallback } = Astro.locals.starlightRoute;
const aiTranslated = entry.data.ai_translated;That would remove the raw-file glob, regex, duplicated locale list, and most of the route-matching logic. It would also make it easier to avoid showing the translation notice on an English fallback page under a localized URL. One smaller thing is that the same Finally, I would either localize the banner text or at least add But again, this is based on a quick look while I'm away from my PC, so definitely double-check the technical details above. Overall I really like this approach. The visual design is unobtrusive, the session-only dismissal is a sensible choice, and having this centralized is far more maintainable than putting notices manually into every document. |
|
@SoongVilda Thank you very much for the detailed feedback! I wasn't aware that a banner was already used, have I always ignored it or does it not render correctly live? Anyway, I'm also not on my PC rn but I'll look into how to implement your changes tomorrow and give a more detailed answer! |
|
@SoongVilda Alright, I've tried to implement your suggested changes where possible and the logic is now much less janky. The main fixes are that, similar to your suggestions, I added the ai_translated flag to content.config.ts so it doesn't search the text anymore and it reads the locale from Astro.locals.starlightRoute instead of analyzing the path. Further it now closes and stops rendering on reloads in a more reliable way and even differenciates for both types of banners so at minimum you have to X the normal one and X the AI one once per session. |
|
@cantosun99 From what I can see, you addressed basically everything I mentioned: the original Starlight banner is preserved, "ai_translated" is part of the schema now, locale/fallback handling uses Starlight directly, Pagefind is ignored, the two banner types have separate dismissal states, and "lang="en"" is there as well. I don't see anything here anymore that I would consider blocking the PR. The only tiny thing I can still think of is that the translation banner starts with "display: none" and is made visible by JS, so if JS or "sessionStorage" fails it would stay hidden. Doing it the other way around, visible by default and letting JS hide it when already dismissed, would be slightly more robust from a progressive-enhancement perspective. But I'd consider that optional rather than something worth holding the PR over. For a modern docs site this is a pretty minor edge case. So yeah, from my side this looks good now. I really like how this ended up, especially compared to having the disclaimer duplicated manually across every translated page. 👍 |
|
@SoongVilda Perfect, I'm happy to hear it! Regarding the rendering, that actually was an intentional decision by me because if it's rendered the other way around, you can see the translation banner pop in and out again for a frame or two which looks awkward. This way, in the worst case, it could potentially hide it, in the best/regular case it looks significantly cleaner, so I'd suggest to keep it this way. Thank you for the help and kind words, I really appreciate it! |
@SoongVilda I think I came up with a pretty elegant solution to the translation disclaimers we discussed in Issue #550.
This PR adds a file src/components/TranslationBanner.astro which makes the disclaimer you wrote appear on all pages that aren't English. If you add a "ai_translated: true" flag to the header of a file, it will instead display the universal disclaimer with the ai generated line in the middle.
Lines 1-12 are vibe-coded, I admit, but the banner was designed by me to look as unintrusive as possible. I matched the existing colors and design so it integrates well into the existing site. Also I wrote the close button to be saved into session storage which is less intrusive than cookies or whatever and lets the disclaimer remain closed on all pages as long as you still have the same browser instance open. If it doesn't get closed, it remains sticky to the header and non-transparent when scrolling.
I already had it translated in every language but then decided to keep it in English for all. Simple reason: If you can't read the English banner, the English wiki will likely not be thaaat useful to you lmao.
I find this to be infinitely less intrusive than a permanent :::note in every page. Also we don't have to change literally every translated file, the banner will be automatically applied and the ai version only on newly translated ones. I already updated the Russian files to go along with this PR.
Examples:
English with no banner:
German with the default banner:
Russian with the ai banner:
German mobile: