Skip to content

feat: wrap UI strings in __() and add RTL support - #2512

Closed
medo94my wants to merge 2 commits into
frappe:developfrom
medo94my:i18n-and-rtl-improvements
Closed

medo94my wants to merge 2 commits into
frappe:developfrom
medo94my:i18n-and-rtl-improvements

Conversation

@medo94my

Copy link
Copy Markdown

What

Follow-up to #2506, split out as requested by @raizasafeel ("Could you raise another PR with just the rest of the fixes?"). The Arabic translation CSV is excluded — translations are contributed via Crowdin. This PR contains only the supporting code fixes that benefit all locales.

Changes

  • Wrap ~95 hardcoded English strings with __() across Settings, Sidebar (AppSidebar, UserDropdown), Statistics, Programming Exercises, Batches, Programs, and other components — so they become translatable in every Crowdin language, not just Arabic.
  • Fix null-language fallback in get_translations() (lms/lms/api.py): a logged-in user with no language set previously passed null to get_all_translations(). It now falls back to System Settings, then "en". The duplicate System Settings lookup is collapsed into a single query.
  • RTL support in index.css: html[dir='rtl'] font-family override, preserving monospace fonts for code/pre blocks — benefits any RTL locale (ar/he/fa/ur).
  • Load IBM Plex Sans Arabic for proper RTL rendering.

Addresses prior review

The two P1 items the automated review raised on #2506 are already handled here:

  • Settings.vue doctype is a plain string (ref('LMS Settings')), not wrapped in __() — so the DocType lookup is unaffected.
  • The translation-key whitespace issue was specific to ar.csv, which is not part of this PR.

Testing

Deployed and tested on a live instance: Desk sidebar, LMS frontend, onboarding, and settings render correctly, and RTL layout applies as expected.

Improves i18n coverage so strings flow through to all locales via Crowdin:

- Wrap ~95 hardcoded English strings with __() across Settings, Sidebar,
  Statistics, Programming Exercises, Batches and other components
- Fix null-language fallback in get_translations(): logged-in users with
  no language set now fall back to System Settings, then "en" (previously
  passed null to get_all_translations); collapses the duplicate System
  Settings lookup into a single query
- Add RTL font-family override (html[dir='rtl']) in index.css, preserving
  monospace fonts for code/pre blocks
- Load IBM Plex Sans Arabic for proper RTL rendering

Translations themselves are contributed via Crowdin, not included here.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@greptile-apps

greptile-apps Bot commented Jun 27, 2026 •

Copy link
Copy Markdown
Contributor

Confidence Score: 5/5

This looks safe to merge.

  • No blocking issues found in the changed code.

Important Files Changed

Filename Overview
frontend/src/components/Settings/Settings.vue Wraps more settings text for translation while keeping stable lookup values unchanged.
lms/lms/api.py Adds a fallback language path when the user or system language is empty.
frontend/src/index.css Adds RTL font rules while preserving monospace rendering for code blocks.

Reviews (2): Last reviewed commit: "fix: keep settings item labels in Englis..." | Re-trigger Greptile

Comment thread frontend/src/components/Settings/Settings.vue
Settings item labels double as selection keys: get_translations() open
path matches `item.label === settingsStore.activeTab` and SidebarLink
compares `link.label.includes(activeTab)` against stable English names.
Wrapping the labels in __() broke section selection in non-English
locales. SidebarLink and SettingDetails already translate labels at
display time via __(link.label) / __(label), so the data-level labels
should stay English. Unwraps the three affected item labels
(Course Progress, Email Templates, Google Meet).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@raizasafeel

Copy link
Copy Markdown
Contributor

Thanks for sending this separately, could you rebase on top of latest develop (11 conflicts, and LiveClassModal.vue/BulkCertificates.vue are both deleted upstream now). Also change these:

  1. or "en" in get_translations() — we already have that in the backend via frappe.local.lang, its not needed. Just call get_all_translations(frappe.local.lang), same as get_all_translations already guards the null case. Also this whole function is basically reimplementing what frappe/translate.py already does (form dict → cookie → header → user → system settings), so we lose the guest language detection by hardcoding it.

  2. MobileLayout.vue — addLink(__('Log in'), 'LogIn') breaks the button. handleClick compares tab.label == 'Log in', so once it's translated that check fails and the click does nothing. Same as the bug in f925137 — revert to addLink('Log in', 'LogIn') and translate at render ({{ link.label }}) instead. Same issue applies to isVisible() and filterLinksToShow(), both match/slugify on tab.label — check those too.

  3. Split the RTL font stuff into its own PR, it's unrelated to the __() work and needs a different approach anyway — see 4 and 5.

  4. Drop the Google Fonts links in index.html. Loads on every page/locale for a font only RTL uses, and it's a third-party request self-hosted/air-gapped instances shouldn't need.

  5. Same for the html[dir='rtl'] * block in index.css — don't fight Tailwind's rtl:/logical-properties setup with a separate !important rule. It also breaks font-mono for the code submission textarea (ProgrammingExerciseSubmission.vue:73), which is the one place we really don't want a proportional font. Better to just add IBM Plex Sans Arabic to the sans stack in tailwind.config.js and let the browser fall back per-glyph — no dir check needed, and the font links go away too.

  6. AppSidebar.vue — leave __('Frappe Learning') untranslated, it's a product name, same class of bug as RFC: User Profile #2 if it ever gets matched against English somewhere.

  7. Prettier's failing on Settings.vue, SettingFields.vue, ProgrammingExercises.vue — lines are past 80 chars now. Needs to be clean before CI will pass. Please read frappe's contribution guidelines and lint your code accordingly

@sashalab

Copy link
Copy Markdown

Additional confirmation for the null-language case on a self-hosted Frappe v16 deployment with social-login provisioning: an authenticated user with an unset User.language can receive an empty translation dictionary even when the site language is Russian. The current LMS main at 4b73070 still reads User.language directly, while Frappe version-16 at 5cba016e returns {} for an empty language. This is separate from missing translation strings. The proposed use of frappe.local.lang in the maintainer review would address the fallback without requiring per-user backfills. A regression test covering a logged-in user with no explicit language and a non-English system language would be valuable.

@raizasafeel

Copy link
Copy Markdown
Contributor

Closing this due to inactivity, please reopen with requested changes

@raizasafeel raizasafeel closed this Sep 7, 2026
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.

3 participants