-
Notifications
You must be signed in to change notification settings - Fork 92
feat: add RTL support to AppFooter
#318
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.
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
AppFooter - add RTL supportAppFooter
AppFooterAppFooter
| {{ $t('footer.docs') }} | ||
| <span class="i-carbon-launch w-3 h-3" aria-hidden="true" /> | ||
| <span>{{ $t('footer.docs') }}</span> | ||
| <span class="i-carbon:launch rtl-flip w-3 h-3" aria-hidden="true" /> |
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.
out of curiosity, why change the class name of the icon?
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.
parsing logic with - separator for collection and icon can be expensive; using : the collection and the icon in just one pass: i-carbon-x-y can be carbon or carbon-x collection and x-y or y icon respectivelly
| <footer class="border-t border-border mt-auto"> | ||
| <div class="container py-3 sm:py-8 flex flex-col gap-2 sm:gap-4 text-fg-subtle text-sm"> | ||
| <div class="flex flex-col sm:flex-row items-center justify-between gap-2 sm:gap-4"> | ||
| <div class="flex flex-col sm:flex-row items-center justify-start gap-2 sm:gap-4"> |
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 curious about the change away from justify-content: space-between. How does that improve RTL support? From my understanding, in flexbox layout, the main axis will flip when the direction changes. Is there something else I’m missing?
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.
Because we need to swap the left and the right inner containers => links on left for LTR and on right for RTL, I added a span between. With justify-between both container without gap (1fr between)
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.
Sorry, I don’t follow. Flexbox is already writing-mode/directionality aware, so the layout contents would have already flipped before this PR (i.e. “links on left for LTR and on right for RTL” already happens). Using justify-content: start has no effect on the directionality of the items within the layout (flex-direction could have that effect, but it’s also the wrong tool for the task) and only seems to introduce the problem that requires the empty, flex-growing/shrink, ARIA hidden <span> element hack.
Usually, I wouldn’t call this out, but since it’s propagating a hack everywhere, I think it’s worth considering if it’s actually necessary. IMO the only changes relevant to RTL support in this PR is adding the rtl-flip class to the icons and changing text-left to text-start in a couple of places.
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.
We need to keep the html markup: A <---> B. If we want to visually change on RTL to B <---> A using justify-between, how can we do that? We need correct html markup and we need to swap containers.
How do you display an icon + button or button + icon? adding justify-start with correct html markup, on RTL we need to change the "order" (justify-start will do for us) => here we want to have a separator between them and so I added an span.
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.
Check https://npmxdev-git-fork-userquin-app-footer-rtl-poetry.vercel.app/ adding dir="rtl" to the html in the devtools and check the links container and the paragraph below.
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.
justify-content has no effect on the order of flex items — only the placement of items on the main axis of a flex layout (e.g. justify-content: start will have items flow from the start of the main axis). There is no need to change the order with CSS or change the markup. Like I said, flexbox (and even grid) are writing-mode/directionality aware and will change how items flow within an axis based on the directionality of the context. Internationalization has already been baked into these layouts from the start.
See: https://rtlstyling.com/posts/rtl-styling/#flexbox-layout-module
A little demo: https://knowler.dev/demos/KeLDPt7?codepen
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.
can you provide the alternative? I want to understand how to solve it, I'm not saying your're wrong, get the original AppFooter.vue and play with it (and maybe send a PR), I'm applying the same "hack" in another components/pages (when using justify-between).
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.
PR here: #340
It just reverts all of the extra spans, that hacky span, and any introduction of justify-start.
| <div class="flex flex-col sm:flex-row items-center justify-start gap-2 sm:gap-4"> | ||
| <p class="font-mono m-0 hidden sm:block">{{ $t('tagline') }}</p> | ||
| <div class="flex items-center gap-3 sm:gap-6"> | ||
| <span aria-hidden="true" class="flex-shrink-1 flex-grow-1" /> |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
In npmx-dev#318, `justify-content: space-between` was removed unnecessary and a hack was introduced in its place. Flexbox layout already is directionality/writing-mode aware and `justify-content` has no bearing on the order of items, just their placement within the “main” axis. This switches back to what it was before which is simpler as it doesn’t add extra elements, presentational elements that are hidden with ARIA.


releated to #317