jorgebucaran / classcat

Build a class attribute string quickly
MIT License
905 stars 22 forks source link

When all child elements are `false` the object key is still included #13

Closed developerdizzle closed 6 years ago

developerdizzle commented 6 years ago

Eg:

classcat([
  "tab",
  {
    tab: {
      "--success": false,
    }
  }
]); // "tab tab"

https://codepen.io/dizzle/pen/baOjjj

I would expect it not to be included in the class list, but is this by design?

jorgebucaran commented 6 years ago

@developerdizzle I think this should be considered a bug, and only one tab should be included.

developerdizzle commented 6 years ago

I added this to the codepen, but I found a workaround:

classcat([
  {
    tab: {
      " ": true,
      "--success": false,
    }
  }
]); // "tab "

Also noticed that classNames has a dedupe version

jorgebucaran commented 6 years ago

@developerdizzle You are right this is a case of duplication, but we should be able to take care of it without "deduping" the entire result. This is also harmless by the way.

To clarify, I don't want to support a dedupe version like in classNames, but I'd like to try to fix this particular issue.

developerdizzle commented 6 years ago

Agreed; I think a full dedupe is overkill. I'll see if I can spend some time on it tomorrow and hopefully find a concise solution.

developerdizzle commented 6 years ago

@JorgeBucaran I think I'm close, but this test is failing https://github.com/JorgeBucaran/classcat/blob/master/tests/index.test.js#L37-L49

Is this a legit use-case? I don't see it in the documentation and it seems to go against being a unary function.

jorgebucaran commented 6 years ago

Hmm, it looks like a perfectly valid used case. 🤔

developerdizzle commented 6 years ago

Is it supposed to be documented somewhere?

jorgebucaran commented 6 years ago

It's definitely not well documented.

developerdizzle commented 6 years ago

@JorgeBucaran https://github.com/JorgeBucaran/classcat/pull/14 simplifies some things, and fixes this bug, but it does break that case. Curious to hear what you think.

developerdizzle commented 6 years ago

I think part of the complication is that prefix is used for both the separator between class names, and the prefix for child elements of an object.

jorgebucaran commented 6 years ago

Are you using this feature? I have to admit I wish I hadn't added it. 😅

developerdizzle commented 6 years ago

The "prefix" feature whose test results in "foo-bar-baz" ? I am not! And honestly, if I was, I'd expect it to be a true prefix for each class (not a separator), eg: "-foo -bar -baz"

developerdizzle commented 6 years ago

I'm not sure how many implementations it would break by removing it, though... 😟

jorgebucaran commented 6 years ago

Exactly, people seem to have different expectations for what this should or shouldn't do. It was easy to support prefixes / BEM style class when I first made classcat, but now I think I should have waited to see if there was a real need.

Well, there's always 2.0.

developerdizzle commented 6 years ago

I'll leave this and the PR open, but feel free to request changes or reject it as a whole.