korbit-ai[bot] commented on code in PR #36100:
URL: https://github.com/apache/superset/pull/36100#discussion_r2523377124
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/ActionButtons/index.tsx:
##########
@@ -110,6 +121,17 @@ const ActionButtons = ({
filterBarOrientation = FilterBarOrientation.Vertical,
chartCustomizationItems,
}: ActionButtonsProps) => {
+ // Check if locale is Persian (fa) for RTL support
+ const isRTL = useMemo(() => {
+ // Check document direction
+ if (typeof document !== 'undefined' && document.documentElement) {
+ return document.documentElement.dir === 'rtl' ||
+ document.documentElement.lang === 'fa' ||
+ document.documentElement.lang?.startsWith('fa');
+ }
+ return false;
+ }, []);
Review Comment:
### RTL Detection Logic Should Be Extracted <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
RTL detection logic is tightly coupled with the component and contains
hardcoded language checks specific to Persian (fa). This violates the Single
Responsibility Principle and makes the code less maintainable and reusable.
###### Why this matters
If RTL requirements change or new languages need RTL support, changes would
be required in multiple places. The component is also taking on the
responsibility of language/direction detection which should be handled at a
higher level.
###### Suggested change ∙ *Feature Preview*
Create a custom hook or utility function for RTL detection:
```typescript
const useRTL = () => {
return useMemo(() => {
return i18n.getDirection() === 'rtl';
}, [i18n.language]);
};
```
Then in the component:
```typescript
const isRTL = useRTL();
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bfec9fd1-1d23-4aa9-9675-06ee3331c5bc/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bfec9fd1-1d23-4aa9-9675-06ee3331c5bc?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bfec9fd1-1d23-4aa9-9675-06ee3331c5bc?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bfec9fd1-1d23-4aa9-9675-06ee3331c5bc?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bfec9fd1-1d23-4aa9-9675-06ee3331c5bc)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:fe14f980-e085-49ff-9084-204a8e5e7421 -->
[](fe14f980-e085-49ff-9084-204a8e5e7421)
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/ActionButtons/index.tsx:
##########
@@ -110,6 +121,17 @@ const ActionButtons = ({
filterBarOrientation = FilterBarOrientation.Vertical,
chartCustomizationItems,
}: ActionButtonsProps) => {
+ // Check if locale is Persian (fa) for RTL support
+ const isRTL = useMemo(() => {
+ // Check document direction
+ if (typeof document !== 'undefined' && document.documentElement) {
+ return document.documentElement.dir === 'rtl' ||
+ document.documentElement.lang === 'fa' ||
+ document.documentElement.lang?.startsWith('fa');
+ }
+ return false;
+ }, []);
Review Comment:
### Incomplete RTL language detection <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The RTL detection logic is hardcoded to only check for Persian language
('fa') and doesn't support other RTL languages like Arabic ('ar'), Hebrew
('he'), or Urdu ('ur').
###### Why this matters
This will cause incorrect layout behavior for users of other RTL languages,
as the component will render in LTR mode instead of RTL mode for non-Persian
RTL languages.
###### Suggested change ∙ *Feature Preview*
Replace the hardcoded Persian check with a comprehensive RTL language
detection:
```typescript
const isRTL = useMemo(() => {
// Check document direction
if (typeof document !== 'undefined' && document.documentElement) {
const rtlLanguages = ['ar', 'he', 'fa', 'ur', 'ps', 'sd', 'ku', 'dv'];
const lang = document.documentElement.lang?.toLowerCase();
return document.documentElement.dir === 'rtl' ||
(lang && rtlLanguages.some(rtlLang => lang.startsWith(rtlLang)));
}
return false;
}, []);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4bd8f541-9d62-4cdf-819c-14b5a54a90b3/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4bd8f541-9d62-4cdf-819c-14b5a54a90b3?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4bd8f541-9d62-4cdf-819c-14b5a54a90b3?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4bd8f541-9d62-4cdf-819c-14b5a54a90b3?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4bd8f541-9d62-4cdf-819c-14b5a54a90b3)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:37bdfc0c-30ce-42cc-9920-2926dd7d7e69 -->
[](37bdfc0c-30ce-42cc-9920-2926dd7d7e69)
--
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]
