-
Notifications
You must be signed in to change notification settings - Fork 2
feat: right to left layout #101
Changes from 8 commits
69c8430
3c3df1b
b93d76c
b417604
e7b7971
9ea9f81
677325c
5b3b20b
5eafd6e
b354101
8059a89
cbefafa
22ef376
5f95063
6ee336a
1e67d95
c89a0cc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
{ | ||
"en": "English", | ||
"es": "Español" | ||
} | ||
"es": "Español", | ||
"ar": "اَلْعَرَبِيَّةُ" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,7 @@ export interface VerseSelectorProps { | |
} | ||
|
||
export function VerseSelector({ verseId, onVerseChange }: VerseSelectorProps) { | ||
const { t } = useTranslation(['translation', 'bible']); | ||
const { t, i18n } = useTranslation(['translation', 'bible']); | ||
const verseInfo = parseVerseId(verseId); | ||
|
||
const onKeyDown = (e: KeyboardEvent<HTMLInputElement>) => { | ||
|
@@ -34,22 +34,26 @@ export function VerseSelector({ verseId, onVerseChange }: VerseSelectorProps) { | |
}; | ||
|
||
return ( | ||
<div className="flex flex-row gap-4 items-center"> | ||
<div className="flex gap-4 items-center flex-row"> | ||
<TextInput | ||
name="verseReference" | ||
autoComplete="off" | ||
placeholder={generateReference(verseInfo, t)} | ||
onKeyDown={onKeyDown} | ||
arial-label={t('select_verse')} | ||
/> | ||
<button onClick={() => onVerseChange(decrementVerseId(verseId))}> | ||
<Icon icon="arrow-left" /> | ||
<span className="sr-only">{t('previous_verse')}</span> | ||
</button> | ||
<button onClick={() => onVerseChange(incrementVerseId(verseId))}> | ||
<Icon icon="arrow-right" /> | ||
<span className="sr-only">{t('next_verse')}</span> | ||
</button> | ||
{/* Use flex-row-reverse on rtl, so that the previous button is always | ||
to the left of the next button. */} | ||
<div className="flex ltr:flex-row rtl:flex-row-reverse gap-4"> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In a RTL language, we want the next button to be on the right. It's confusing for us, but natural in those languages. We should be able to remove this secondary flex box and leave it in the natural rtl order, but flip the icons. Might be easiest to render two icons and show/hide the correct one with tailwind rather than trying to manage that with react There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wondered if this was the case or not There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It just looked so strange to me 😆 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
<button onClick={() => onVerseChange(decrementVerseId(verseId))}> | ||
<Icon icon="arrow-left" /> | ||
<span className="sr-only">{t('previous_verse')}</span> | ||
</button> | ||
<button onClick={() => onVerseChange(incrementVerseId(verseId))}> | ||
<Icon icon="arrow-right" /> | ||
<span className="sr-only">{t('next_verse')}</span> | ||
</button> | ||
</div> | ||
</div> | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,23 +1,30 @@ | ||
import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; | ||
import { StrictMode } from 'react'; | ||
import * as ReactDOM from 'react-dom/client'; | ||
import { useTranslation } from 'react-i18next'; | ||
import { RouterProvider } from 'react-router-dom'; | ||
import router from './app/router'; | ||
import './app/i18n'; | ||
import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; | ||
import router from './app/router'; | ||
import { FlashProvider } from './shared/hooks/flash'; | ||
|
||
function App() { | ||
const { i18n } = useTranslation(); | ||
document.body.dir = i18n.dir(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should only update this when the language changes. We can use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So I should create a local "dir" variable, update it in the event callback, and watch that variable in the useEffect? (I'm still trying to get comfortable with the react way of doing things) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Something like this should work: useEffect(() => {
function handler() {
document.body.dir = i18next.dir()
}
i18next.on('initialized', handler)
i18next.on('languageChanged', handler)
}, [i18next.dir, i18next.on]) That will attach event listeners to i18next and only apply changes when the language changes. Both the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried this, but the handler never got called. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem is that both events trigger before the callback of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think my newest commit fixes it. Instead of using the |
||
return ( | ||
<StrictMode> | ||
<QueryClientProvider client={queryClient}> | ||
<FlashProvider> | ||
<RouterProvider router={router} /> | ||
</FlashProvider> | ||
</QueryClientProvider> | ||
</StrictMode> | ||
); | ||
} | ||
|
||
const root = ReactDOM.createRoot( | ||
document.getElementById('root') as HTMLElement | ||
); | ||
|
||
const queryClient = new QueryClient(); | ||
|
||
root.render( | ||
<StrictMode> | ||
<QueryClientProvider client={queryClient}> | ||
<FlashProvider> | ||
<RouterProvider router={router} /> | ||
</FlashProvider> | ||
</QueryClientProvider> | ||
</StrictMode> | ||
); | ||
root.render(<App />); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this library might not be the best supported, but it does what it does well. I think it is easier to read and write
me-4
thanltr:ml-4 rtl:mr-4
, especially when we will have that kind of thing all throughout the codebase. Thestart-[x]
andend-[x]
utilities are also very useful, instead ofltr:left-[x] rtl:right-[x]
.The only bug I really ran into was that the library doesn't handle negative numbers. 20lives/tailwindcss-rtl#53. It does look like the maintainer has quite releasing updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm in total agreement about the ease of these new utilities. However, given that we've already run into a limitation of this unmaintained library, what do you think about using the code from the library directly in our tailwind config? Since we are going to be heavily using direction utilities, maybe we should have control over them. Here is all that it would take to add margin utilities, including negative margins. That just wasn't released in this library. We wouldn't need to add everything the library supports to start, just the ones that we need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't even think of this. Good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I brought over all the utilities we are using.