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

Support of Tailwind Arbitrary Classes #906

Open
snguyenG2 opened this issue Feb 9, 2023 · 14 comments
Open

Support of Tailwind Arbitrary Classes #906

snguyenG2 opened this issue Feb 9, 2023 · 14 comments

Comments

@snguyenG2
Copy link

snguyenG2 commented Feb 9, 2023

Can we get support for Tailwind Arbitrary classes?

Ex:

div[class='p-1 border border-midnight-20 rounded-b-[3px]'] works fine

but if we did .p-1.border.border-midnight-20.rounded-b-[3px] slim will not parse correctly.

@airblade
Copy link

airblade commented Mar 1, 2023

As a first step I removed square brackets from attr_list_delims:

Slim::Engine.options[:attr_list_delims] = {
  '(' => ')',
  '{' => '}'
}

But it still doesn't work. For example: .font-["foo"] hello produces <div class="font-">["foo"] hello</div>.

@bb
Copy link

bb commented Mar 1, 2023

You need at least also remove them from code_attr_delims if I understand https://github.com/slim-template/slim/blob/main/lib/slim/parser.rb#L81 correctly. But I used those delims too often, so I didn't try further.

@airblade
Copy link

airblade commented Mar 1, 2023

@bb Thanks, but but it still doesn't work.

@bb
Copy link

bb commented Mar 1, 2023

Yeah, sorry. Maybe I'll give it another try later but no plans for now.

@bb
Copy link

bb commented Mar 1, 2023

I couldn't stop myself and gave it another look: We'll need to change https://github.com/slim-template/slim/blob/main/lib/slim/parser.rb#L73 from

@attr_shortcut_re = /\A(#{keys}+)((?:\p{Word}|-|\/\d+|:(\w|-)+)*)/

to include [ and ] but only if they're not used in attr_list_delims. While changing that, it'd be wise to also include ( and ) because they'll be needed for theme and calc anyway.

From what I can see right now, code_attr_delims doesn't matter, it's only used after end of attribute which cannot happen at these positions, so my previous comment was actually misleading.

@airblade
Copy link

airblade commented Mar 1, 2023

I couldn't stop myself and gave it another look

Haha, I know that feeling :)

@attr_shortcut_re = /\A(#{keys}+)((?:\p{Word}|-|\/\d+|:(\w|-)+)*)/

I was looking at that line too but didn't understand it fully enough to suggest any changes. I'm happy to help test though.

@bb
Copy link

bb commented Mar 1, 2023

So setting

Slim::Engine.options[:attr_list_delims] = {
  '{' => '}'
}

and and changing Line 73 to
@attr_shortcut_re = /\A(#{keys}+)((?:\p{Word}|-|\[|\]|\(|\)|\/\d+|:(\w|-)+)*)/

actually makes

.foo-[a(b)].bar()
compile to
<div class="foo-[a(b)] bar()"></div>

and

.foo-[a(b)].bar() {realarg="value"} content
to
<div class="foo-[a(b)] bar()" realarg="value">content</div>

That worked for me for a quick test, but actually, we can't just change L73 as I did above but need to add each (actual) letter out of \[|\]|\(|\)| to the regex only if it's not used in attr_list_delims.

As we're here anyway: we also need : without the special rule (last part) for stuff like lg:[&:nth-child(3)]:hover:underline. And also &.

So now I'm at
@attr_shortcut_re = /\A(#{keys}+)((?:\p{Word}|-|\[|\]|\(|\)|:|&|\/\d+)*)/
and
.foo-[a(b)].bar().lg:[&:nth-child(3)]:hover:underline {realarg="value"} content
which successfully translates to
<div class="foo-[a(b)] bar() lg:[&:nth-child(3)]:hover:underline" realarg="value">content</div>

And it still isn't conditional on attr_list_delims and I don't know why usage of : was constrained to be only followed by letters or -, so it may break something... and I'm also not sure if we covered all characters needed for Tailwind arbitrary classes yet.

@airblade
Copy link

airblade commented Mar 1, 2023

Great progress!

Can it handle quotation marks in the arbitrary values, for stuff like after:content-['_↗'] (from here) or font-['Open_Sans'] (from here)?

@bb
Copy link

bb commented Mar 1, 2023

Neither worked with the previous state.
As we're basically all-in with before content and after content, I now converted it to a negative selection. This also already includes forbidding the attr_list_delims.

      keys = Regexp.union @attr_shortcut.keys.sort_by {|k| -k.size }
      non_attr_chars = Regexp.escape ('"></='.split(//) + @attr_list_delims.flatten + @attr_shortcut.keys).uniq.join
      @attr_shortcut_re = /\A(#{keys}+)([^\0\s#{non_attr_chars}]*)/

.lg:[&:nth-child(3)]:hover:underline.after:content-['_↗'].font-['Open_Sans']{realarg="value"} content

<div class="lg:[&:nth-child(3)]:hover:underline after:content-['_↗'] font-['Open_Sans']" realarg="value">content</div>

All the tests are still passing... so... I guess I could as well create a PR (after creating new tests for the changed code).

@airblade
Copy link

airblade commented Mar 2, 2023

non_attr_chars = Regexp.escape ('"></='.split(//) + @attr_list_delims.flatten + @attr_shortcut.keys).uniq.join

I see that / is included here. Does that prevent classes like w-1/2?

@bb
Copy link

bb commented Mar 2, 2023

You're right. I did some additional changes:
Removed (duplicate) < and > because in some tests they were used as delimiters which produced warnings.
Needed to forbid : and / at the end, so I needed to add a negative look-behind. Those are needed for nested tag lines and self-closing tags respectively.

Also needed to forbid the dynamic splat_prefix (default *) which needs special handling compared to the other non_attribute_chars because the prefix may consist of multiple characters. The test suite uses those: ['*','**','*!','*%','*^','*$']

So this is the result:

      non_attr_chars = Regexp.escape (@attr_list_delims.flatten + @attr_shortcut.keys).uniq.join.delete("<", ">")
      splat_prefix = Regexp.escape(options[:splat_prefix])
      @attr_shortcut_re = /\A(#{keys}+)((?:(?!#{splat_prefix})[^\0\"><\=\s#{non_attr_chars}])*)(?<![:\/])/

The line assigning splat prefix was just moved up from the bottom of the function.

Finally, this is the additional test:

  def test_tailwindcss_arbitrary_classes
    source = %(
.lg:[&:nth-child(3)]:hover:underline.after:content-['_↗'].font-['Open_Sans']#the-id arg="value" Test it
)
    assert_html '<div arg="value" class="lg:[&:nth-child(3)]:hover:underline after:content-[\'_↗\'] font-[\'Open_Sans\']" id="the-id">Test it</div>', source, attr_list_delims: { '{' => '}' }
  end

I was afraid the whole thing might be a bit slower because of the two additional lookarounds, but I didn't see a benchmark to try it. Just looking at assertions/s from the test suite, they seem to be equal.

I also noticed yet another thing which is not supported this way: classes mx-0.5 because . is used as a class delimiter. We might want to allow it when it's prefixed with \. I didn't continue on that yet.

However, I noticed another issue: Tailwinds content-class-finder (previously pruner) doesn't detect classes which start with . AND contain [ or ] (and possibly others) at the same time.

So, if I have

ul{class=""}
  li.lg:[&:nth-child(3)]:hover:underline.after:content-['_↗'].font-['Open_Sans'] Hello
  li.lg:[&:nth-child(3)]:hover:underline.after:content-['_↗'].font-['Open_Sans'] Hello
  li.lg:[&:nth-child(3)]:hover:underline.after:content-['_↗'].font-['Open_Sans'].font-bold Hello
  li.lg:[&:nth-child(3)]:hover:underline.after:content-['_↗'].font-['Open_Sans'].bg-cyan-300 Hello

I see font-bold and bg-cyan-300 as generated css classes but not the others.
When I use lg:[&:nth-child(3)]:hover:underline somewhere else Tailwind recognizes that it is needed and it generates it and it works here as well.
This may need some changes in Tailwind itself.

@airblade
Copy link

airblade commented Mar 3, 2023

Well done getting all that to work.

Tailwinds content-class-finder (previously pruner) doesn't detect classes which start with . AND contain [ or ] (and possibly others) at the same time.

Tailwind's test for slim templates doesn't check classes containing [ or ]:

https://github.com/tailwindlabs/tailwindcss/blob/994b541779fdc82b15d67b27b290820e37e94733/tests/default-extractor.test.js#L458-L470

I cloned the Tailwind repo and tried to experiment but I couldn't get the tests to run at all.

This may need some changes in Tailwind itself.

Sounds like it.

@bb
Copy link

bb commented Mar 4, 2023

Running the tailwindcss tests worked fine for me. After cloning and npm install I could run the whole suite using npm run test or just that file using jest tests/default-extractor.test.js or just this test jest tests/default-extractor.test.js --testNamePattern slim.

Looking at this commit: https://github.com/tailwindlabs/tailwindcss/pull/8569/files#diff-4b945b54be020aaf62bfce1f632eb903751230a871fa1e87aa6b7f95c15b3d9dR94 - the "inner matches" regex in line 94 (which is line 119 in master) seems to be the one responsible for slim to work... but it also leads to overgenerating CSS. You can see that in the tests where "not contain pr-1" was removed after expecting "pr-1.5".

I tried to modify it and also to duplicate it and additionally have similar rule to work with the new test cases I added, but I couldn't get it to work yet. Also, changes like adding : in the left part of the rule led to significant overgenerating (i.e. for all examples with utility prefixes, also the base version was generated, e.g. hover:underline does not need to generate .underline without hover (and maaany more cases like this)...

So, I tried another path and basically changed all positions where a backtick was added to also add the dot. This broke all existing tests which contained a decimal value. Third variation: duplicate all those rules, one with and one without the dot. You see, I didn't even try to understand all of it. I was just pattern matching the match patterns 😃

This lead to only 8 failed tests, those where all others.
Then I added yield /[^<>"'\s(){}[]#=%$]*[^<>"'\s.(){}[\]#=%:$]/g below the "inner matches` (this is the same but removes the dot from the first part). With this, only 3 failed, but one of the 3 was my own... and I didn't even try every combination yet but started with simplified examples.

I'm afraid it will be really, really complex to both split at . and keep . within decimals, within arbitrary values, within before/after-content strings, within theme-functions etc. -- I think it wouldn't matter to overgenerate class tokens because if they're not valid tailwind syntax, they'll not overgenerate too much CSS, but still.

I wonder if our mighty overlord @adamwathan would want to take a look here 👀.

This is the current state: wip.patch
Sorry, my time slot is over now. 🛌

@airblade
Copy link

airblade commented Mar 5, 2023

@bb Phew, that's a lot of work!

Perhaps a simpler approach would be for Slim to require "special" characters to be escaped in class names when used in a . class shortcut. For example, .mx-0\.5 or .font-\['Open_Sans'\].

It would require updating both Slim and Tailwind, but hopefully just to match (and ignore) a backslash here and there. Well, it sounds straightforward :)

In the meantime, div class="mx-0.5" Something works of course and is still less noisy than HTML's <div class="mx-0.5">Something</div>.

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

No branches or pull requests

3 participants