EnxDev commented on PR #34629:
URL: https://github.com/apache/superset/pull/34629#issuecomment-4809398265
## EnxDev's Review Agent โ apache/superset#34629 ยท HEAD 430eb59
request changes โ RTL plumbing is solid and well-tested, but the
document-direction write skips first paint, locale parsing misses Superset's
underscore form, and several `DirectionType` value-imports/non-RTL CSS changes
need cleanup. CI is fully red and the PR needs a rebase.
### ๐ด Functional
- **`Theme.tsx:242-249`** โ `<html dir>`/`data-direction` are written only
inside the reassigned `updateProviders`, which isn't called on initial mount.
On first paint the document direction isn't applied; it currently gets
corrected only because `LanguagePicker`'s effect later calls `setDirection`, so
any view without the language picker never sets it (and RTL users see a flash).
Move the attribute write into a `useEffect(..., [direction])` in
`SupersetThemeProvider`? (test: render provider with an rtl theme, assert
`document.documentElement.dir === 'rtl'`)
- **`localeUtils.ts:22`** โ `getLanguageCodeFromLocale` splits on `-` only,
but Superset normalizes locales to underscore form (`pt_BR`, `zh_TW`; `base.py`
does `locale.replace("-", "_")`). A region-suffixed RTL locale like
`ar_SA`/`fa_IR` returns the whole token, misses `rtlLanguages`, and renders
LTR. Split on `/[-_]/`? (test: `getDirectionFromLocale('ar_SA') === 'rtl'` โ
the existing `ar-SA` test doesn't match Superset's real format)
### ๐ก Should-fix
- **`Theme.tsx:30`, `ThemeController.ts:31`, `ThemeProvider.tsx:27`,
`LanguagePicker.tsx:25`** โ `import { DirectionType }` is a runtime import of a
type-only symbol (the other files correctly use `import type`); with
`isolatedModules` + oxlint `consistent-type-imports` this fails lint/build. It
also imports `antd` directly โ reuse the existing `TextDirection` (`'ltr' |
'rtl'`) from `localeUtils` or a `@superset-ui/core` type. (`lint-frontend` was
skipped this run, so CI didn't flag it.)
- **`Menu.tsx:54`, `SubMenu.tsx:88`,
`PageHeaderWithActions/index.tsx:130-145`** โ unconditional `.main-nav {
display: flex }`, `.menu { justify-content: space-between }`, and the
`title-panel`โ`Flex` refactor are layout changes not specific to RTL;
`display:flex` on `.main-nav` can disturb antd's inline/mobile menu mode.
@justinpark already asked to keep non-RTL CSS out โ scope these down or justify
each.
- **`DynamicEditableTitle/index.tsx:64`** โ `opacity: 0` hides the sizer but
it stays interactive (absolute, no `pointer-events`), so it can overlap and
intercept clicks/selection on the title. `visibility: hidden` avoids the `left:
-9999px` horizontal scroll *and* disables pointer events while still allowing
`offsetWidth` measurement.
### ๐ต Nits
- `LanguagePicker.tsx:42` โ the direction-setting `useEffect` inside
`useLanguageMenuItems` duplicates
`ThemeController.initializeDirectionFromLocale`; a "get menu items" hook now
mutates global theme as a side effect. Consider centralizing direction in the
controller.
- `constants.ts:305` โ `rtlLanguages` is an untyped `string[]`; `as const`
communicates intent and narrows the type.
### ๐ Praise
- Good unit coverage โ `localeUtils`, `ThemeController`, `LanguagePicker`,
and `Theme.setDirection` persistence are all tested.
- LTR/RTL Emotion caches via `stylis-plugin-rtl`, keyed by direction, is a
clean approach: physical CSS props flip automatically without per-component
edits.
<!-- enxdev-review-agent:430eb59 -->
_Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD 430eb59._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
