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

Proposed ability to add sub & pseudo classes #93

Closed
wants to merge 2 commits into from
Closed

Proposed ability to add sub & pseudo classes #93

wants to merge 2 commits into from

Conversation

SirCameron
Copy link

Instead of writing:
hover:foo hover:bar hover:baz hover:after:foo hover:after:baz

This proposes:
clsx({ hover: ["foo", "bar", "baz", { after: ["foo", "baz"] }] });

Please excuse the reformatting of code :)

Copy link
Owner

@lukeed lukeed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change, which means it cannot proceed. clsx is heavily depended on & is feature-complete. Any behavior modifications would have to be surfaced as a separate, standalone module (eg, clsx/lite).

Also, on a general note, please try to refrain from code-style changes as it makes PRs harder to review.

Comment on lines -83 to -87
emptyObject: {},
nonEmptyObject: {a: 1, b: 2},
emptyList: [],
nonEmptyList: [1, 2, 3],
greaterZero: 1
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing these isn't a fix. The next assertion fails w/ these restored, which shows that this PR is a breaking change.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I didn't propose it as a non-breaking change. You are already doing versioned releases; this would be a new breaking release, no?

I removed those because they no longer apply as truthy - not as a fix, but a change.

I tried to keep the code style changes to a min.

@lukeed
Copy link
Owner

lukeed commented Mar 25, 2024

Closing this PR for the reason mentioned above. You may open an issue to propose a clsx/nested (🤷 name tbd) submodule and invite discussion. I think this could be useful to some, but it definitely can't be added as the default behavior.

Thank you for the work & suggestion!

@lukeed lukeed closed this Mar 25, 2024
@SirCameron
Copy link
Author

SirCameron commented Mar 25, 2024

Thanks for checking it out :) I'll put it in a submodule and create a new PR.

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

2 participants