lukeed/clsxPublic

A tiny (239B) utility for constructing `className` strings conditionally.

Proposed ability to add sub & pseudo classes #93

ClosedSirCameronclosed 2 commits intomasterfromrefs/pull/93/head
  1. SI
    SirCameron

    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 :)

  2. SIadded subclass and pseudo class functionalitybf915ed
  3. SIfixed quotes05202c6
  4. LU
    lukeed(edited)

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

    test/classnames.js:83–87base05202c6
  5. LU
    lukeedChanges requested

    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.

  6. SI
    SirCameron
    Author

    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.

    test/classnames.js:83–87base05202c6
  7. SI
    SirCameronReviewed
    Author

    No review summary.

  8. LU
    lukeed

    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!

  9. lukeedclosed2 years ago on GitHub

  10. SI
    SirCameron(edited)
    Author

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