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

Allow replacing of previous class names #18

Closed
longlho opened this issue Mar 23, 2015 · 13 comments · Fixed by #20
Closed

Allow replacing of previous class names #18

longlho opened this issue Mar 23, 2015 · 13 comments · Fixed by #20

Comments

@longlho
Copy link
Contributor

longlho commented Mar 23, 2015

My use case is something like classNames('foo', { foo: false, bar: true }) should render out class="bar" instead of class="foo bar". Should this be supported by the library or is this out of scope? I'd be glad to submit a PR if this use case is something you guys plan to support.

@cakesifu
Copy link

👍 on this.

@JedWatson
Copy link
Owner

As long as there isn't a negative performance impact, I think this is a reasonable change to the API.

@dcousens
Copy link
Collaborator

I personally don't think de duplication is the responsibility of this library, given it is entirely a user input problem.

@JedWatson
Copy link
Owner

Can someone put forward the use-case that is driving this requirement? (I mean big picture, not simple example)

Looking at the complexity of implementation I'm inclined to agree with @dcousens

@longlho
Copy link
Contributor Author

longlho commented Mar 30, 2015

So my use case is that a React component which uses this library to allow class name overwrites w/ a specific set of default classNames, so something like classNames('foo', extraStuff).

Conceptually since this library allows a chaining/combining a set of class names, specifying {foo: false} at the end of the chain and have it still appear in the output makes it very hard to debug, as you basically have to crawl down the chain of abstractions and figure out which layer injected foo in. And since foo will always be there you need to have hacky CSS rule to make certain selectors more specific and/or throw !important in there.

@cakesifu
Copy link

I agree with @dcousens but I also have 2 compelling reasons for adding this in anyway:

  1. In order to do the de-duplication outside of the library one would have to parse the inputs and do most of the work that the library already does.
  2. Allowing later arguments to overwrite previous arguments is more idiomatic and almost expected behavior (think _.extend).

@longlho
Copy link
Contributor Author

longlho commented Mar 30, 2015

@cezar-berea We're having some discussion in the PR itself. @JedWatson mentioned exposing .dedupe (or .merge if we wanna be similar to _.merge) and I think that's probably best as an opt-in due to performance impact

cakesifu added a commit to cakesifu/classnames that referenced this issue Mar 31, 2015
@dcousens
Copy link
Collaborator

@JedWatson what is your conclusion here, .dedupe?

@3den
Copy link

3den commented Apr 30, 2015

can someone get that merged?

@JedWatson
Copy link
Owner

@3den will land it this weekend.

@dcousens
Copy link
Collaborator

dcousens commented May 3, 2015

@JedWatson as .dedupe?

@dcousens
Copy link
Collaborator

dcousens commented May 3, 2015

Closing this in favour of #31

@dcousens dcousens closed this as completed May 3, 2015
@JedWatson
Copy link
Owner

Just released 2.1.0 with this included.

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 a pull request may close this issue.

5 participants