fix: change to css logical props #379

lord007tn · 2024-03-05T08:25:29Z

I did change to css logical props
add rotation in rtl from chevrons and arrows
except the one in the onKeyDown function in carousel as it require a testing on what direction the website is
i belive this will change after we settle on the RTL/LTR provider for components

i changed text to be on start rather than forced to be left as it's more meaningful to what direction the text is written

sadeghbarati · 2024-03-05T09:11:33Z

With this, we don't need another CLI feature for converting ltr (directional properties) to rtl (directional properties)

We can also showcase ConfigProvider, rtl and ltr in shadcn-vue site

CSS Logical Properties support table is nice and most Vite projects are for modern browsers so we are safe to merge this

zernonia · 2024-03-05T10:01:21Z

Im not sure if everyone would like this feature tho. having to change the mindset of pl to ps is not easy, and might affect the productivity somehow.

This sounds more like a opt-in feature to me. 😅

Saeid-Za · 2024-03-05T10:33:25Z

The alternative would be adding a CLI option with semi-automated for replacing the classes, but it could not be fully automated (some classes have exceptions and have only one directionality). this means the burden of creating multiple version of components would be on maintainers' shoulders.
we already have default & new-york going forward we'd also need RTL versions.

I understand that this creates mental burden for the end user but I had personally forked this repository months ago specially to address multi direction support. (same approach from PR)
What are your thoughts? 👀

Saeid-Za · 2024-03-05T10:53:12Z

One other note worth mentioning, this approach is by far the best way to support multi-direction IMO, for reference, here are some alternatives that are just not as good:

Providing manual multiple versions of the style
Using postcss transformer in build step
Quasar uses this approach and the plugin is postcss-rtl

sadeghbarati · 2024-03-05T10:57:37Z

How we can make it an opt-in feature?
I think it's better to have multiple registries then

default directional properties TS
default directional properties JS

default logical properties TS
default logical properties JS

Related issue #378

lord007tn · 2024-03-05T12:28:21Z

As someone who works with rtl and ltr on daily basis in b2c and b2b products
i think that logical CSS props are most useful in supporting multi-direction websites better than generating new CSS using plugins like postcss-rtl
by default HTML have a LTR direction so we have a default in case your website will be in LTR website
if you are going to support rtl we always tend to change lang and dir in top <html> tag
and boom we have a rtl direction now
using libraries that uses css properties that force direction like margin-left, text-left
will be a headache to manage and that's a no no to use
as we see css logical props don't cause problems for LTR to use and support RTL also so a win win

zernonia · 2024-03-05T14:59:49Z

Quick question! how does logical properties works with tailwind-merge? 🤔

Would it have conflicts if we add pl-* to ps-*, which one would it resolves?

sadeghbarati · 2024-03-05T15:49:51Z

@zernonia

JSPM Playground

tailwind-merge will resolve when both classes are of the same kind

twMerge('pl-4', 'ps-6') ---> pl-4 ps-6 (directional + logical)
twMerge('pl-4', 'pl-6') ---> pl-6 (directional + directional)
twMerge('ps-4', 'ps-6') ---> ps-6 (logical+ logical)

Saeid-Za · 2024-03-05T17:14:19Z

It seems that according to unocss playground, the output css for this html:

<div class="ms-10 ml-1"></div>

Is in this order:

/* layer: default */
.ml-1{margin-left:0.25rem;}
.ms-10{margin-inline-start:2.5rem;}

Which means, logical classes always win by default.
But regarding the tailwind-merge behavior, what is the correct answer in logical-directional situations anyway?

The true answer depends on the current direction of the web page. TM cannot remove ms-10 from calling twMerge('ms-10', 'ml-1') because in RTL direction it changes its behavior.

Well, this is getting complicated 😅

Saeid-Za · 2024-03-06T13:08:21Z

Update:
There is a response from maintainer of tailwind-merge:

Hey @lord007tn! 👋

tailwind-merge doesn't resolve conflicts between normal and logical properties because it doesn't have access to the axis along which logical properties are being applied. This axis is dynamic and can be different for any node in the DOM.

E.g. ps-6 (applying the padding-inline-start property) would be in conflict with pl-4 without any other properties being set. But if a parent node applies the CSS direction: rtl, ps-6 wouldn't be in conflict with pl-4 anymore because ps-6 then gets applied to the right side instead. And if additionally writing-mode: vertical-lr is applied (to a parent), ps-6 gets applied to the bottom of the node.

Considering this is the expected behavior, this is not an issue of whether user could understand logical properties, this PR enforces that ALL users must use logical instead of directional (or normal) properties for their style to work.

I'm fully team logical here, but this is an unexpected chain of changes. it seems the only valid solution is to provide an other registry for Logical values, so the user understands that they need to work with logical props all the time. nice catch @zernonia.

Anything that I missed here?

lord007tn and others added 6 commits March 5, 2024 08:56

fix: change to css logical props

2b6dfd1

fix: change space-x-* with gap and add flex

71fea4f

fix: add rotation on rtl for chevrons and arrows

71d97fa

fix: change text-left to text-start to support rtl written text

7dfd0d4

fix: change text-left to text-start to support rtl written text

9def0db

chore: update

523270e

sadeghbarati requested a review from zernonia March 5, 2024 09:07

zernonia linked an issue Mar 5, 2024 that may be closed by this pull request

[Feature]: Support RTL in shadcn components #357

Open

5 tasks

lord007tn mentioned this pull request Mar 5, 2024

logical css properties seems not to resolve when conflicting with normal css properties dcastil/tailwind-merge#388

Open

sadeghbarati mentioned this pull request Mar 21, 2024

[Feature]: Addressing the limitations for [RTL] layouts out of the box #421

Closed

2 tasks

Gruce mentioned this pull request Apr 16, 2024

kit-ui/shadcn-ui EnabApp/EnabApp#22

Open

55 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: change to css logical props #379

fix: change to css logical props #379

lord007tn commented Mar 5, 2024

sadeghbarati commented Mar 5, 2024 •

edited

zernonia commented Mar 5, 2024 •

edited

Saeid-Za commented Mar 5, 2024

Saeid-Za commented Mar 5, 2024

sadeghbarati commented Mar 5, 2024

lord007tn commented Mar 5, 2024

zernonia commented Mar 5, 2024

sadeghbarati commented Mar 5, 2024 •

edited

Saeid-Za commented Mar 5, 2024 •

edited

Saeid-Za commented Mar 6, 2024 •

edited

fix: change to css logical props #379

Are you sure you want to change the base?

fix: change to css logical props #379

Conversation

lord007tn commented Mar 5, 2024

sadeghbarati commented Mar 5, 2024 • edited

zernonia commented Mar 5, 2024 • edited

Saeid-Za commented Mar 5, 2024

Saeid-Za commented Mar 5, 2024

sadeghbarati commented Mar 5, 2024

lord007tn commented Mar 5, 2024

zernonia commented Mar 5, 2024

sadeghbarati commented Mar 5, 2024 • edited

Saeid-Za commented Mar 5, 2024 • edited

Saeid-Za commented Mar 6, 2024 • edited

sadeghbarati commented Mar 5, 2024 •

edited

zernonia commented Mar 5, 2024 •

edited

sadeghbarati commented Mar 5, 2024 •

edited

Saeid-Za commented Mar 5, 2024 •

edited

Saeid-Za commented Mar 6, 2024 •

edited