feat(i18n): add RTL text direction and typography support for Persian and Arabic#4075
feat(i18n): add RTL text direction and typography support for Persian and Arabic#4075MRGhust wants to merge 2 commits into
Conversation
…port for Persian and Arabic
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
WalkthroughThis PR centralizes RTL language codes, applies or removes an ChangesRTL Language Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/client/LangSelector.ts (1)
214-220: ⚡ Quick winExtract the RTL logic to avoid duplication.
The same RTL detection and class/attribute update logic appears in both
initializeLanguage(lines 103-109) andchangeLanguage(lines 214-220). Extract this into a helper method to keep the code DRY.♻️ Suggested refactor
Add a private helper method:
+ private applyRtlSettings(lang: string): void { + const isRtl = RTL_LANGUAGES.has(lang); + if (isRtl) { + document.documentElement.classList.add("rtl-text"); + } else { + document.documentElement.classList.remove("rtl-text"); + } + document.documentElement.setAttribute("lang", lang); + }Then use it in both places:
this.defaultTranslations = defaultTranslations; this.translations = translations; this.currentLang = userLang; - const isRtl = RTL_LANGUAGES.has(userLang); - if (isRtl) { - document.documentElement.classList.add("rtl-text"); - } else { - document.documentElement.classList.remove("rtl-text"); - } - document.documentElement.setAttribute("lang", userLang); + this.applyRtlSettings(userLang); await this.loadLanguageList();localStorage.setItem("lang", lang); this.translations = await this.loadLanguage(lang); this.currentLang = lang; - const isRtl = RTL_LANGUAGES.has(lang); - if (isRtl) { - document.documentElement.classList.add("rtl-text"); - } else { - document.documentElement.classList.remove("rtl-text"); - } - document.documentElement.setAttribute("lang", lang); + this.applyRtlSettings(lang); this.applyTranslation();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/LangSelector.ts` around lines 214 - 220, Extract the duplicated RTL handling in initializeLanguage and changeLanguage into a private helper (e.g., applyRtlForLang or updateRtlForLang) that takes the lang string, sets document.documentElement.lang attribute, checks RTL_LANGUAGES.has(lang) and adds/removes the "rtl-text" class accordingly; then replace the RTL/class/attribute lines in initializeLanguage and changeLanguage with calls to that helper to keep logic DRY and centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/styles.css`:
- Around line 42-48: Remove the invalid element selector "html.rtl-text
font-mono" from the selector list—keep the class-based selector ".font-mono" and
the other valid selectors ("html.rtl-text .font-mono", "html.rtl-text kbd",
"html.rtl-text code") so the rule applies correctly; update the selector list in
the block that currently starts with "html.rtl-text font-mono" to exclude the
erroneous element selector.
---
Nitpick comments:
In `@src/client/LangSelector.ts`:
- Around line 214-220: Extract the duplicated RTL handling in initializeLanguage
and changeLanguage into a private helper (e.g., applyRtlForLang or
updateRtlForLang) that takes the lang string, sets document.documentElement.lang
attribute, checks RTL_LANGUAGES.has(lang) and adds/removes the "rtl-text" class
accordingly; then replace the RTL/class/attribute lines in initializeLanguage
and changeLanguage with calls to that helper to keep logic DRY and centralized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 606ed08f-1976-4a4b-9c58-b615e4eed567
📒 Files selected for processing (2)
src/client/LangSelector.tssrc/client/styles.css
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
MRGhust
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a703996410
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| html.rtl-text .font-mono, | ||
| html.rtl-text kbd, | ||
| html.rtl-text code { | ||
| direction: ltr; | ||
| display: inline-block; |
There was a problem hiding this comment.
When Persian, Arabic, or Hebrew is selected, this selector applies display: inline-block to every .font-mono element, not just inline key labels/code. Several existing controls combine .font-mono with layout utilities such as flex (for example the keybind button in src/client/components/baseComponents/setting/SettingKeybind.ts), so this higher-specificity rule overrides their flex display and removes the centering/alignment behavior in RTL mode. Limit the display override to actual inline kbd/code elements or preserve the existing display for .font-mono containers.
Useful? React with 👍 / 👎.
1 participant
Description:
This Pull Request introduces non-destructive, class-based Right-to-Left (RTL) text flow and premium typography support for Persian (
fa), Arabic (ar), and Hebrew (he) language elements.Instead of full-document mirroring (which would break or mirror the global WebGL game coordinate layouts, main top bars, sidebars, and in-game controls), this solution targets only readable text containers (paragraphs, tables, lists, headers, and prose elements) and sets their text flow and alignment under a dynamic
.rtl-textCSS class.Key Improvements:
.rtl-textand standardlangattributes dynamically insideLangSelector.tsbased on language selection..rtl-textinsidestyles.cssfor clean Arabic/Persian/Hebrew glyph rendering..pl-4and.pl-5styles in RTL mode and remaps them to right paddings to align bullet points and table cells perfectly on the right.Esc,Ctrl,Shift) to prevent symbol inversion.All 1,300+ automated unit tests passed successfully, and ESLint / Prettier formatting checks passed with zero warnings.
Discord Username: NOBODYiran