Copilot commented on code in PR #34629:
URL: https://github.com/apache/superset/pull/34629#discussion_r2267506250
##########
superset-frontend/src/constants.ts:
##########
@@ -190,3 +190,16 @@ export enum Actions {
CREATE = 'create',
UPDATE = 'update',
}
+
+export const rtlLanguages = [
+ 'ar',
+ 'fa',
+ 'he',
+ 'iw',
+ 'kd',
+ 'pk',
Review Comment:
The language code 'pk' appears to be a country code (Pakistan) rather than a
language code. RTL detection should use language codes like 'ur' for Urdu.
Consider reviewing and correcting any country codes in the rtlLanguages array.
```suggestion
```
##########
superset-frontend/packages/superset-ui-core/src/theme/Theme.tsx:
##########
@@ -241,18 +262,23 @@ export class Theme {
const [themeState, setThemeState] = React.useState({
theme: this.theme,
antdConfig: this.antdConfig,
- emotionCache: createCache({ key: 'superset' }),
+ emotionCache: this.createCache(),
});
+ const { direction = 'ltr' } = themeState.theme;
this.updateProviders = (theme, antdConfig, emotionCache) => {
setThemeState({ theme, antdConfig, emotionCache });
+ if (theme.direction === 'rtl') {
+ document?.documentElement?.setAttribute('dir', 'rtl');
+ document?.documentElement?.setAttribute('data-direction', 'rtl');
+ }
};
Review Comment:
The RTL direction is only applied to document attributes when direction is
'rtl', but there's no handling for when direction changes back to 'ltr'. This
could leave stale RTL attributes on the document. Consider also handling the
'ltr' case to remove RTL attributes.
```suggestion
} else {
document?.documentElement?.setAttribute('dir', 'ltr');
document?.documentElement?.setAttribute('data-direction', 'ltr');
}
```
##########
superset-frontend/src/features/home/LanguagePicker.tsx:
##########
@@ -56,11 +60,20 @@ const StyledFlag = styled.i`
`;
export default function LanguagePicker(props: LanguagePickerProps) {
- const { locale, languages, ...rest } = props;
+ const { locale, languages, setDirection, ...rest } = props;
const theme = useTheme();
+
+ useEffect(() => {
+ const isRtl = rtlLanguages.some(l => locale.startsWith(l));
Review Comment:
The RTL detection logic using `locale.startsWith(l)` could produce false
positives. For example, if locale is 'arabic' and rtlLanguages contains 'ar',
this would incorrectly match. Consider using exact locale matching or splitting
the locale by '-' and checking the language code part.
```suggestion
const languageCode = locale.split('-')[0];
const isRtl = rtlLanguages.includes(languageCode);
```
##########
superset-frontend/src/constants.ts:
##########
@@ -190,3 +190,16 @@ export enum Actions {
CREATE = 'create',
UPDATE = 'update',
}
+
+export const rtlLanguages = [
+ 'ar',
+ 'fa',
+ 'he',
+ 'iw',
+ 'kd',
Review Comment:
The language code 'kd' is not a standard ISO 639-1 language code. Consider
verifying all language codes in the rtlLanguages array against standard
language codes to ensure compatibility with internationalization libraries.
```suggestion
```
--
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]
