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 Glimmer Concatenation #119

Merged

Conversation

Alonski
Copy link
Contributor

@Alonski Alonski commented Jan 30, 2023

Glimmer supports concatenation via the (concat) helper.

The plugin is currently incorrectly breaking up the concatenation

I first added a failing test showing the issue.

Use ignoreLast in a Glimmer StringLiteral if the parent is a SubExpression (meaning helper) and the last character of the node value is not whitespace

Meaning (concat "border-l-blue border-" @color) does not get sorted

While (concat "border-l-blue border ") does get sorted

Glimmer supports concatenation via the `(concat)` helper.

The plugin is currently incorrectly breaking up the concatenation

This adds a failing test showing the issue.
src/index.js Outdated Show resolved Hide resolved
@thecrypticace
Copy link
Contributor

I think, for now at least, we should special-case concat as an exception. Though we made end up changing this to all sub-expressions but I think that might be far too broad of an approach. Could you update the PR to do that?

Use ignoreLast in a Glimmer StringLiteral if the parent is a SubExpression (meaning helper) and the last character of the node value is not whitespace

Meaning `(concat "border-l-blue border-" @color)` does not get sorted

While `(concat "border-l-blue border ")` does get sorted
The main issue is with (concat) so we will hardcode that in

If in the future we think we need more helpers we can do that
@Alonski Alonski force-pushed the bugfix/no-fix-concat-glimmer branch from a1a1946 to 4183b28 Compare February 5, 2023 13:38
@Alonski
Copy link
Contributor Author

Alonski commented Feb 6, 2023

@thecrypticace Can you take another look please?

@thecrypticace thecrypticace self-assigned this Feb 7, 2023
@thecrypticace thecrypticace merged commit e14a94b into tailwindlabs:main Feb 7, 2023
@thecrypticace
Copy link
Contributor

Thanks! Will get a release out this week with this change. Appreciate it! ✨

@Alonski
Copy link
Contributor Author

Alonski commented Feb 7, 2023

Thank you! Can't wait!

@Alonski Alonski deleted the bugfix/no-fix-concat-glimmer branch February 8, 2023 08:21
@Alonski
Copy link
Contributor Author

Alonski commented Feb 15, 2023

Hey @thecrypticace,
Can you please release a new version with this fix?

@thecrypticace
Copy link
Contributor

@Alonski Ah sorry! This one slipped by me. Just published v0.2.3 on NPM

@Alonski
Copy link
Contributor Author

Alonski commented Feb 15, 2023

Thanks! @MichalBryxi let's update our repo!

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.

None yet

3 participants