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

Support a prefix for classes instead of just leaving them out #17

Closed
rmoorman opened this issue Mar 22, 2015 · 9 comments
Closed

Support a prefix for classes instead of just leaving them out #17

rmoorman opened this issue Mar 22, 2015 · 9 comments

Comments

@rmoorman
Copy link

Hello there,

I would like to suggest a small addition to the functionality of this library. It would be handy IMHO if there would be a way to add a prefix (such as "no-") instead of just leaving the class out of the generated class string.

so that next to this:

classNames({ foo: true }, { bar: true }); // => 'foo bar'
classNames({ foo: true }, { bar: false }); // => 'foo'

the something like the following would also be possible:

classNamesPrefixed("no-")({ foo: true }, { bar: true }); // => 'foo bar'
classNamesPrefixed("no-")({ foo: true }, { bar: false }); // => 'foo no-bar'

Best regards,
Rico Moorman

@JedWatson
Copy link
Owner

Interesting idea, I think there may be a cleaner way to achieve this though; something that preprocesses objects beforehand.

If you've got { "foo": true, "bar": false }
you want to turn it into { "foo": true, "no-bar": true }
then it can be passed in to classNames as-is.

@rmoorman
Copy link
Author

That would match with what I meant. I really don't think that the main modules public interface should be adjusted but that an additional function could be available who maps to the classNames one. One can then just use a different require to use the different functionality.

I am not sure what would be the nicest function interface for this one though ... the one described initially would then just return a function which just maps over it's arguments and adds a prefixed version for each unprefixed key and then feeds the result into classNames. The arguments to classnames would then be something like this: {"foo": value1, "no-foo": !value1, "bar": value2, "no-bar": !value2}.
What do you think?

@mtscout6
Copy link

What about another type of prefixing that's not a falsey prefix. For example:

// needs a better name...
classNamesPrefixedNotFalsy("herp-")({ foo: true, bar: true }); // => 'herp-foo herp-bar'

This would be an extremely useful use case for me in react-bootstrap. react-bootstrap/react-bootstrap#404 is one example. Note that I have applied a rather crude means to accomplish this here.

@rmoorman
Copy link
Author

That seems a nice use case too. Unconditional prefixing. We need more names :-). Maybe we could generalize this into a formatting option for the list and integrate with some formatting options available (maybe someone wants prefixing ... someone wants suffixing ...)

@rmoorman
Copy link
Author

@mtscout6 if the classnames would be available as a list rather than a string, wouldn't the prefixing be trivial by mapping over it?
@JedWatson , wouldn't using a list structure as a base make #18 more prettier/simpler to implement? You would then just have to throw one function into the mix which deduplicates a list/make the items unique. After that you would just have to join the items with a space.

@JedWatson
Copy link
Owner

@rmoorman we used an Array in previous versions but string concatenation provided an incredible improvement to execution speed. I'm normally all for flexibility in my packages but this one has become the de facto replacement for React's classnames utility and as such I'm being very careful to preserve performance. Also, you're likely to see a considerable number of executions in your app, so it has to scale.

@mtscout6 I get where you're coming from with classes in react-bootstrap. What if we added two methods, like this:

  • prefix(obj, truthyPrefix[, falsyPrefix]) - prefixes truthy keys in obj with the truthyPrefix, optionally flips and prefixes falsy keys in obj with falsyPrefix if it is provided
  • prefix.falsy(obj, falsyPrefix) - flips and prefixes falsy keys in obj with falsyPrefix
var classNames = require('classnames');
var prefix = require('classnames/prefix');

var c = { a: true, b: false };
classNames(prefix(c, 'pre-')); // "pre-a"
classNames(prefix.falsy(c, 'not-')); // "a not-b"
classNames(prefix(c, 'is-', 'not-')); // "is-a not-b"

Sounds like this would meet all the requirements I've heard without impacting the performance of complexity of classNames itself.

Thoughts?

@rmoorman
Copy link
Author

@JedWatson I can see where you are coming from wrt. execution performance. As far as flexibility is concerned, the proposed solution could indeed provide a lot of it (also for suffixing, general formatting). It looks like the prefix and prefix.falsy would need to know a lot about the implementation details of classNames though (given that the list is passed in as parameter to the prefix functions). How about passing in a function as first parameter to classnames which then takes care of formatting one each item?

var classNames = require('classnames');
var prefix = require('classnames/prefix');

var c = { a: true, b: false };
classNames(prefix("pre-"), c); // "pre-a"
classNames(prefix.falsy("not-"), c); // "a not-b"
classNames(prefix("is-", "not-"), c); // "is-a not-b"

The prefix function (or formatter for that matter) then just needs to receive the current item of the c list while classNames is iterating over it's arguments.

Or maybe a little cleaner to implement, preserving the current public interface of classnames

var classNames = require('classnames');
var classNamesFormat = require('classnames/format');
var prefix = require('classnames/prefix');

var c = { a: true, b: false };
classNames(c); // "a"
classNamesFormat(prefix("pre-"), c); // "pre-a"
classNamesFormat(prefix.falsy("not-"), c); // "a not-b"
classNamesFormat(prefix("is-", "not-"), c); // "is-a not-b"

edit: add example for not changing interface of the classnames function

@dcousens
Copy link
Collaborator

Perhaps it might be easier to make prefix work on the output, rather than the input.

@dcousens
Copy link
Collaborator

This is something that can be done by the user.

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

No branches or pull requests

4 participants