feat: right to left layout #101
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
package.json
Outdated
| @@ -65,6 +65,7 @@ | |||
| "react-i18next": "12.2.0", | |||
| "react-refresh": "0.10.0", | |||
| "tailwindcss": "3.2.7", | |||
| "tailwindcss-rtl": "^0.9.0", | |||
There was a problem hiding this comment.
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 than ltr:ml-4 rtl:mr-4, especially when we will have that kind of thing all throughout the codebase. The start-[x] and end-[x] utilities are also very useful, instead of ltr: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.
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.
I didn't even think of this. Good idea.
There was a problem hiding this comment.
I think I brought over all the utilities we are using.
There was a problem hiding this comment.
I didn't even try to translate the short versions, I didn't know where to start. We definitely will want an actual Arabic speaker to update all the translations in this file at some point.
There was a problem hiding this comment.
Most of the name translations are computer generated. I went back and replaced a few from this list.
There was a problem hiding this comment.
We can remove the short versions for now. In situations where we would use t('gen', { context: 'short' }) it will fallback to the normal key if the _short key is missing.
# Conflicts: # packages/web/src/features/languages/LanguagesView.tsx
packages/web/src/main.tsx
Outdated
| import { FlashProvider } from './shared/hooks/flash'; | ||
|
|
||
| function App() { | ||
| const { i18n } = useTranslation(); | ||
| document.body.dir = i18n.dir(); |
There was a problem hiding this comment.
We should only update this when the language changes. We can use useEffect to set up event listeners to i18next events
There was a problem hiding this comment.
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.
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 dir and on functions should be stable (meaning their reference doesn't change), so this effect will fire only once on first render and attach the listeners.
There was a problem hiding this comment.
I tried this, but the handler never got called.
There was a problem hiding this comment.
The problem is that both events trigger before the callback of useEffect is called.
There was a problem hiding this comment.
I think my newest commit fixes it. Instead of using the initialized event, I just call the handler directly from the effect.
There was a problem hiding this comment.
We can remove the short versions for now. In situations where we would use t('gen', { context: 'short' }) it will fallback to the normal key if the _short key is missing.
There was a problem hiding this comment.
These locale json files have been updated in the past few PRs. Can you make sure the arabic ones are up to date?
| {/* 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.
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.
I wondered if this was the case or not
There was a problem hiding this comment.
It just looked so strange to me 😆
package.json
Outdated
| @@ -65,6 +65,7 @@ | |||
| "react-i18next": "12.2.0", | |||
| "react-refresh": "0.10.0", | |||
| "tailwindcss": "3.2.7", | |||
| "tailwindcss-rtl": "^0.9.0", | |||
There was a problem hiding this comment.
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.
|
I noticed this when preparing my update video: When in a RTL language, we still want the Hebrew translation line to be RTL. Right now, they are flipped back to LTR |
Thanks for catching that! I remember noticing this, but then I forgot to address it. |
|
I think I addressed everything you brought up. I'll be offline until Wednesday. |
To Do
tailwindcss-rtlwith alternative method