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

:not([dir="rtl"]) is breaking everything #44

Closed
cdes opened this issue Jan 13, 2022 · 6 comments
Closed

:not([dir="rtl"]) is breaking everything #44

cdes opened this issue Jan 13, 2022 · 6 comments

Comments

@cdes
Copy link

cdes commented Jan 13, 2022

the change you did in the 0.8.0 is not working as you would expect. You can see a screen shot here where :not([dir="rtl"]) and [dir="rtl"] are both valid.

Although [dir="rtl"] matches the tag selector, :not([dir="rtl"]) will match the selector.

Screen Shot 2022-01-02 at 5 50 04 PM

@Reex11
Copy link

Reex11 commented Jan 24, 2022

I think the issue is that the rule :not([dir="rtl"]) is for the current element (regardless of the parent) so I think it would be better if we assigned specified it to the body body:not([dir="rtl"])

@Reex11
Copy link

Reex11 commented Jan 24, 2022

I created a PR #45 to fix this issue. It will depend on the body direction.
Check it out and let me know what you think.

@vltansky
Copy link
Contributor

@20lives merge please

@StormVanDerPol
Copy link

maybe it's better to revert to dir="ltr" and just require either ltr or rtl on the top level layout element of your choosing, since #45 locks it to the body element, which may go against some people their use case.

e.g. @cdes in his screenshot uses the html element. Others may place the dir attribute further down the tree.

@20lives
Copy link
Owner

20lives commented Jan 27, 2022

Hi everyone!
Sorry for my unavailability, was occupied with other things.

The last change did break a lot of stuff and I am sorry for that as well.

I think that reverting to the last version is the best decision (and "forcing" you to have dir="ltr/rtl" at the top level) is our best choice.

this will be done today, including a new release.

if you think that the other solution that came up is better, let's talk about it.

@20lives
Copy link
Owner

20lives commented Feb 5, 2022

Finally fixed, use version 0.9.0.

@20lives 20lives closed this as completed Feb 5, 2022
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 a pull request may close this issue.

5 participants