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.

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

fix: change to css logical props #379

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from
Open

Conversation

lord007tn
Copy link

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
Copy link
Collaborator

sadeghbarati commented Mar 5, 2024

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
Copy link
Member

zernonia commented Mar 5, 2024

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
Copy link
Contributor

Saeid-Za commented Mar 5, 2024

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
Copy link
Contributor

Saeid-Za commented Mar 5, 2024

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
Copy link
Collaborator

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
Copy link
Author

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 zernonia linked an issue Mar 5, 2024 that may be closed by this pull request
5 tasks
@zernonia
Copy link
Member

zernonia commented Mar 5, 2024

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
Copy link
Collaborator

sadeghbarati commented Mar 5, 2024

@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
Copy link
Contributor

Saeid-Za commented Mar 5, 2024

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
Copy link
Contributor

Saeid-Za commented Mar 6, 2024

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: Support RTL in shadcn components
4 participants