Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improve RTL mode #625

Merged
merged 2 commits into from
Nov 21, 2024
Merged

improve RTL mode #625

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
53988ec
improve RTL mode
chiragchhatrala Nov 21, 2024
0783eb3
Merge branch 'main' into improve-rtl-mode
JhumanJ Nov 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion client/app.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,10 @@ export default {
type: 'image/png',
href: '/favicon.ico'
}
]
],
htmlAttrs: () => ({
dir: 'ltr'
})
Comment on lines +99 to +102
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

⚠️ Potential issue

Critical: Hardcoded LTR direction conflicts with RTL support

The review comment is correct. The evidence shows:

  1. The app has RTL support infrastructure (tailwind config has RTL-aware variants)
  2. The app uses a store (useAppStore is referenced)
  3. Yet the direction is hardcoded to 'ltr' instead of being dynamic

The suggested fix to make it dynamic based on the store's isRTL value is the right approach:

      htmlAttrs: () => ({
-        dir: 'ltr'
+        dir: computed(() => useAppStore().isRTL ? 'rtl' : 'ltr')
      })
🔗 Analysis chain

Critical: Hardcoded LTR direction contradicts RTL support objective

The PR aims to improve RTL mode support, but this change hardcodes the direction to 'ltr' which prevents proper RTL rendering for right-to-left languages (e.g., Arabic, Hebrew).

Consider making the direction dynamic:

       htmlAttrs: () => ({
-        dir: 'ltr'
+        dir: computed(() => useAppStore().isRTL ? 'rtl' : 'ltr')
       })

Let's verify if other components are properly handling RTL mode:

Let me try another search strategy to verify RTL support:

Let me try one final search to check for any RTL-related state management:

Let me try one final search to check the app configuration and store:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for RTL-related configurations and direction handling
echo "Checking for RTL/direction related code..."
rg -g '*.{vue,js,ts}' -i '(rtl|direction|dir:|\bdir\b)' 

# Check if any components are using isRTL from store
echo "Checking for isRTL usage..."
rg -g '*.{vue,js,ts}' 'isRTL'

# Look for language/locale settings that might affect direction
echo "Checking for locale/language settings..."
rg -g '*.{vue,js,ts}' '(locale|language|lang)'

Length of output: 136205

Script:

#!/bin/bash
# Search for RTL/direction related code in store files
echo "Checking store files for RTL/direction handling..."
rg -g '*store*.{js,ts}' -i '(rtl|direction|dir)'

# Check for any RTL/direction related configuration
echo "Checking configuration files..."
rg -g '*config*.{js,ts}' -i '(rtl|direction|dir)'

# Look for any RTL/direction related utilities or helpers
echo "Checking utility files..."
rg -g '*util*.{js,ts}' -i '(rtl|direction|dir)'

Length of output: 731

Script:

#!/bin/bash
# Search for any state/store management files
echo "Checking for RTL state management..."
rg -g '*.{js,ts,vue}' -B 2 -A 2 'isRTL|useRTL|rtlEnabled|rtlMode'

# Check for any language/locale settings that might control RTL
echo "Checking for locale direction settings..."
rg -g '*.{js,ts,vue}' -B 2 -A 2 'direction.*locale|locale.*direction'

# Check app.vue content more thoroughly
echo "Checking app.vue content..."
cat client/app.vue

Length of output: 131574

Script:

#!/bin/bash
# Check app configuration files for RTL/direction settings
echo "Checking app config files..."
rg -g '*config*.{js,ts,json}' -A 2 -B 2 'rtl|direction|dir'

# Check store/state management files
echo "Checking store files..."
rg -g '*store*.{js,ts,vue}' -A 2 -B 2 'rtl|direction|dir|locale'

# Check app.vue content specifically around the htmlAttrs section
echo "Checking app.vue htmlAttrs section..."
rg -g 'app.vue' -A 5 -B 5 'htmlAttrs'

Length of output: 1224

})

const appStore = useAppStore()
Expand Down
2 changes: 1 addition & 1 deletion client/components/forms/DateInput.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
</div>
<button
v-if="fromDate && !props.disabled"
class="hover:bg-gray-50 dark:hover:bg-gray-900 border-l px-2 flex items-center"
class="hover:bg-gray-50 dark:hover:bg-gray-900 ltr:border-l rtl:border-r px-2 flex items-center"
@click.prevent="clear()"
>
<Icon
Expand Down
8 changes: 4 additions & 4 deletions client/components/forms/MatrixInput.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@
<table class="w-full table-auto">
<thead class="">
<tr>
<th class="text-left p-2 w-auto max-w-xs" />
<th class="ltr:text-left rtl:text-right p-2 w-auto max-w-xs" />
<td
v-for="column in columns"
:key="column"
class="border-l border-gray-300 max-w-24 overflow-hidden"
class="ltr:border-l rtl:border-r rtl:!border-l-0 border-gray-300 max-w-24 overflow-hidden"
>
<div class="p-2 w-full flex items-center justify-center text-sm">
{{ column }}
Expand All @@ -33,15 +33,15 @@
:key="rowIndex"
class="border-t border-gray-300"
>
<td class="text-left w-auto max-w-24 overflow-hidden">
<td class="ltr:text-left rtl:text-right w-auto max-w-24 overflow-hidden">
<div class="w-full p-2 text-sm">
{{ row }}
</div>
</td>
<td
v-for="column in columns"
:key="row + column"
class="border-l border-gray-300 hover:!bg-gray-100 dark:hover:!bg-gray-800"
class="ltr:border-l rtl:border-r rtl:!border-l-0 border-gray-300 hover:!bg-gray-100 dark:hover:!bg-gray-800"
:class="{
'!cursor-not-allowed !bg-gray-200 dark:!bg-gray-800 hover:!bg-gray-200 dark:hover:!bg-gray-800': disabled,
}"
Comment on lines +44 to 47
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Simplify styles and fix potential dark mode conflicts

The current implementation has some potential issues with style specificity and dark mode interactions:

-              class="ltr:border-l rtl:border-r rtl:!border-l-0 border-gray-300 hover:!bg-gray-100 dark:hover:!bg-gray-800"
-              :class="{
-                '!cursor-not-allowed !bg-gray-200 dark:!bg-gray-800 hover:!bg-gray-200 dark:hover:!bg-gray-800': disabled,
-              }"
+              class="ltr:border-l rtl:border-r border-gray-300 dark:border-gray-600 group"
+              :class="{
+                'cursor-not-allowed bg-gray-200 dark:bg-gray-800': disabled,
+                'hover:bg-gray-100 dark:hover:bg-gray-800': !disabled
+              }"

Changes:

  1. Simplified border handling for RTL mode
  2. Removed excessive use of !important
  3. Separated hover states for disabled/enabled states
  4. Added dark mode border color
  5. Used Tailwind's group utility for better style organization
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
class="ltr:border-l rtl:border-r rtl:!border-l-0 border-gray-300 hover:!bg-gray-100 dark:hover:!bg-gray-800"
:class="{
'!cursor-not-allowed !bg-gray-200 dark:!bg-gray-800 hover:!bg-gray-200 dark:hover:!bg-gray-800': disabled,
}"
class="ltr:border-l rtl:border-r border-gray-300 dark:border-gray-600 group"
:class="{
'cursor-not-allowed bg-gray-200 dark:bg-gray-800': disabled,
'hover:bg-gray-100 dark:hover:bg-gray-800': !disabled
}"

Expand Down
4 changes: 2 additions & 2 deletions client/components/forms/PhoneInput.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
v-model="selectedCountryCode"
:class="theme.PhoneInput.countrySelectWidth"
dropdown-class="max-w-[300px]"
input-class="rounded-r-none"
input-class="ltr-only:rounded-r-none rtl:!rounded-l-none"
:data="countries"
:disabled="disabled || countries.length === 1 ? true : null"
:searchable="true"
Expand Down Expand Up @@ -55,7 +55,7 @@
<input
v-model="inputVal"
type="text"
class="inline-flex-grow !border-l-0 !rounded-l-none"
class="inline-flex-grow ltr-only:border-l-0 ltr-only:!rounded-l-none rtl:border-r-0 rtl:rounded-r-none"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Ensure consistent use of !important modifiers in RTL/LTR classes.

While the RTL/LTR implementation is correct, there's an inconsistency in the use of !important modifiers:

  • LTR mode uses !rounded-l-none
  • RTL mode uses rounded-r-none without !

This could lead to styling inconsistencies if there are competing styles.

Apply this change for consistency:

-class="inline-flex-grow ltr-only:border-l-0 ltr-only:!rounded-l-none rtl:border-r-0 rtl:rounded-r-none"
+class="inline-flex-grow ltr-only:border-l-0 ltr-only:!rounded-l-none rtl:!border-r-0 rtl:!rounded-r-none"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
class="inline-flex-grow ltr-only:border-l-0 ltr-only:!rounded-l-none rtl:border-r-0 rtl:rounded-r-none"
class="inline-flex-grow ltr-only:border-l-0 ltr-only:!rounded-l-none rtl:!border-r-0 rtl:!rounded-r-none"

:disabled="disabled ? true : null"
:class="[
theme.PhoneInput.input,
Expand Down
14 changes: 7 additions & 7 deletions client/components/forms/components/VSelect.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@
>
<slot name="placeholder">
<div
class="text-gray-400 dark:text-gray-500 w-full text-left truncate pr-3"
class="text-gray-400 dark:text-gray-500 w-full ltr:text-left rtl:!text-right truncate ltr:pr-3 rtl:pl-3 rtl:!pr-0"
:class="[
{ 'py-1': multiple && !loading },
theme.SelectInput.fontSize
Expand All @@ -76,7 +76,7 @@
</div>
</transition>
</div>
<span class="absolute inset-y-0 right-0 flex items-center pr-2 pointer-events-none">
<span class="absolute inset-y-0 ltr:right-0 rtl:left-0 rtl:!right-auto flex items-center ltr:pr-2 rtl:pl-2 rtl:!pr-0 pointer-events-none">
<Icon
name="heroicons:chevron-up-down-16-solid"
class="h-5 w-5 text-gray-500"
Expand All @@ -85,7 +85,7 @@
</button>
<button
v-if="clearable && showClearButton && !isEmpty"
class="hover:bg-gray-50 dark:hover:bg-gray-900 border-l px-2"
class="hover:bg-gray-50 dark:hover:bg-gray-900 ltr:border-l rtl:!border-l-0 rtl:border-r px-2 flex items-center"
:class="[theme.SelectInput.spacing.vertical]"
@click.prevent="clear()"
>
Expand Down Expand Up @@ -119,12 +119,12 @@
<input
v-model="searchTerm"
type="text"
class="flex-grow pl-3 pr-7 py-2 w-full focus:outline-none dark:text-white"
class="flex-grow ltr:pl-3 ltr:pr-7 rtl:!pr-3 rtl:pl-7 py-2 w-full focus:outline-none dark:text-white"
placeholder="Search"
>
<div
v-if="!searchTerm"
class="flex absolute right-0 inset-y-0 items-center px-2 justify-center pointer-events-none"
class="flex absolute ltr:right-0 rtl:left-0 rtl:!right-auto inset-y-0 items-center px-2 justify-center pointer-events-none"
>
<Icon
name="heroicons:magnifying-glass-solid"
Expand All @@ -134,12 +134,12 @@
<div
v-else
role="button"
class="flex absolute right-0 inset-y-0 items-center px-2 justify-center"
class="flex absolute ltr:right-0 rtl:!right-auto rtl:left-0 inset-y-0 items-center px-2 justify-center"
@click="searchTerm = ''"
>
<Icon
name="heroicons:backspace"
class="h-5 w-5 text-gray-500 dark:text-gray-400"
class="h-5 w-5 rtl:rotate-180 text-gray-500 dark:text-gray-400"
/>
</div>
</div>
Expand Down
1 change: 1 addition & 0 deletions client/components/open/forms/OpenCompleteForm.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
<div
v-if="form"
class="open-complete-form"
:dir="form?.layout_rtl ? 'rtl' : 'ltr'"
:style="{ '--font-family': form.font_family, 'direction': form?.layout_rtl ? 'rtl' : 'ltr' }"
>
<link
Expand Down
2 changes: 1 addition & 1 deletion client/components/open/forms/components/form-components/FormCustomization.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@
:form="form"
class="mt-4"
label="Right-to-Left Layout"
help="Adjusts form layout for right-to-left languages."
help="Adjusts layout for RTL languages"
/>

<EditorSectionHeader
Expand Down
5 changes: 4 additions & 1 deletion client/pages/forms/[slug]/index.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,9 @@ useHead({
content: 'black-translucent'
},
] : {},
script: [{ src: '/widgets/iframeResizer.contentWindow.min.js' } ]
script: [{ src: '/widgets/iframeResizer.contentWindow.min.js' }],
htmlAttrs: () => ({
dir: form.value?.layout_rtl ? 'rtl' : 'ltr'
})
})
</script>
2 changes: 2 additions & 0 deletions client/tailwind.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,8 @@ module.exports = {
plugin(function ({ addVariant }) {
addVariant("between", "&:not(:first-child):not(:last-child)")
addVariant("hocus", ["&:hover", "&:focus"])
// Add a new variant that only applies when there's no RTL parent
addVariant('ltr-only', '&:where(:not([dir="rtl"] *))')
}),
],
}
Loading