-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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 class extraction followed by (
in Slim
#17278
Conversation
result[cursor.pos] = b' '; | ||
bracket_stack.push(cursor.curr); | ||
} | ||
|
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.
idk if it matters but should we worry about bg-red-500/(--my-var)
here too? If so we should do !matches!(cursor.prev, b'-' | b'/')
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.
Okay, answering my own question with a note: I think we can drop that matches check entirely based on the PR desc
This is because the check (after adding the /
) makes these two tests I just wrote pass:
let input = r#"
body.border-(--my-color).p-8(class="\#{body_classes}" data-hotwire-native="\#{hotwire_native_app?}" data-controller="update-time-zone")
"#;
Slim::test_extract_contains(input, vec!["border-(--my-color)", "p-8"]);
let input = r#"
body.border-red-500/(--my-color).p-8(class="\#{body_classes}" data-hotwire-native="\#{hotwire_native_app?}" data-controller="update-time-zone")
"#;
Slim::test_extract_contains(input, vec!["border-red-500/(--my-color)", "p-8"]);
but as you mentioned this is invalid Slim syntax
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.
Yep yep, added an additional test just so that this case is covered: 1a9c242
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.
Alright we do need this for this situation: 645db11
div class="bg-(--my-color) bg-(--my-color)/(--my-opacity)"
There is a situation where classes can exist top-level that are _not_ inside of any type of brackets. E.g.: ``` div class="…" ```
This PR fixes an issue where a class shorthand in Pug followed by a `(` is not properly extracted. ```html <template lang="pug"> .text-sky-600.bg-neutral-900(title="A tooltip") This div has an HTML attribute. </template> ``` The `text-sky-600` is extracted, but the `bg-neutral-900` is not. Fixes: #17313 # Test plan 1. Added test to cover this case 2. Existing tests pass (after a few small adjustments due to _more_ extracted candidates, but definitely not _less_) 3. Verified against the original issue (top is before, bottom is this PR) <img width="1307" alt="image" src="https://github.com/user-attachments/assets/68a0529f-63ad-477d-a342-e3f91c5a1690" /> We had this exact same bug in Slim (#17278). Since Pug, Slim and Haml are the only pre processors we have right now with this dot-separated class notation I also double checked the Haml pre-processor if this is an issue or not (and it's already covered there). <img width="1263" alt="image" src="https://github.com/user-attachments/assets/c658168b-d124-46c9-9ec0-9697151a57bf" />
This PR fixes an issue where using the class shorthand in Slim templates, followed by an
(
results in the last class being ignored.E.g.:
This is because we will eventually extract
p-8
but it's followed by an invalid boundary character(
.To solve this, we make sure to replace the
(
with a space. We already do a similar thing when the classes are followed by an[
.One caveat, we can have
(
in our classes, likebg-(--my-color)
. But in my testing this is not something that can be used in the shorthand version.E.g.:
Compiles to:
So I didn't add any special handling for this. Even when trying to escape the
(
,-
and)
characters, it still doesn't work. E.g.:Compiles to:
Test plan
Fixes: #17277