Ability to handle object with own .toString() method#170
Conversation
|
Why not do this yourself before providing them as input? |
|
From my understanding Moreover without this behaviour you cannot properly use custom generators of class names. Just imagine, you have BEM component in react and have block with, let's say 5 elements. And you will need to call manually Basically this PR just adds consistency in behaviour between this library and React. |
|
@resetko I haven't run into this myself, can you provide the example you speak of? |
|
@dcousens, I can quickly pick a little bit simplified real-world example of BEM usage (ft.com), with one remark that instead of We have markup like this: <header class="o-header">
<div class="o-header__row">
<div class="o-header__container">
<div class="o-header__top-wrapper">
<div class="o-header__top-column o-header__top-column--left">
...
</div>
<div class="o-header__top-column o-header__top-column--center">
...
</div>
<div class="o-header__top-column o-header__top-column--right">
...
</div>
</div>
</div>
</div>
</header>I'm proposing to write something like this: const css = new Bem('o-header');
return (
<header className={css}>
<div className={css.el('row')}>
<div className={css.el('container')}>
<div className={css.el('top-wrapper')}>
<div className={css.el('top-column').mod('left')}>
...
</div>
<div className={css.el('top-column').mod('center')}>
...
</div>
<div className={css.el('top-column').mod('right')}>
...
</div>
</div>
</div>
</div>
</header>
);And your solution will look like the following: const css = new Bem('o-header');
return (
<header className={css.toString()}>
<div className={css.el('row').toString()}>
<div className={css.el('container').toString()}>
<div className={css.el('top-wrapper').toString()}>
<div className={css.el('top-column').mod('left').toString()}>
...
</div>
<div className={css.el('top-column').mod('center').toString()}>
...
</div>
<div className={css.el('top-column').mod('right').toString()}>
...
</div>
</div>
</div>
</div>
</header>
);If you want example out of SemanticUI scope - just imagine that I want to use this library like this: Example of implementation of BEM class you can find in SemanticUI issue which I linked it first comment. |
|
Why not wrap it yourself? function cssel (s) {
return css.el(s).toString()
}That wrapper function would resolve 99% of your issue? |
I guess, this is probably the most convincing point, as you said, supporting I'm willing to say +1, but could you show a performance benchmark result? |
|
Sorry, I meant <div className={classNames(css.el('row'), css.el('search-form'))}>About performance:
Or you want me to run internal benchmark tool on my local node? |
Aye, we have a benchmark suite in repository to test this. |
|
Will do this. Give me some time :) Also added one commit in which I changed the condition to handle inherited Will do benchmarks bit later (tomorrow probably). |
|
Thanks for your effort @resetko , it is appreciated 👍 |
|
So, I've done benchmarking 3 times and got 3 different results, but from what I see it more looks like fluctuations: |
|
Difference appears marginal. ACK from me. |
|
Any plans merging this? :) |
|
My company would also benefit from getting this merged. Are there plans to merge it? |
|
@dcousens When this PR will be merged? We're waiting :) |
|
Can I help in some way to make this happen? |
|
Without objection, I think I'll merge this. |
|
Have you given up on this request? |
|
Thanx :) |
|
Great! Any plans to release it? |
|
Bump Will this be released any time soon? |
This will allow using custom
.toString()methods for objects to generate class names.From a performance standpoint, it's not affecting existing scenarios (scenario with an object containing
.toString()method will be obviously slower a bit because we need to call this function).Here is JSPerf for this: https://jsperf.com/classnames-benchmark-for-tostring
From JSPerf I can see that it's hard to find any performance impact, so I it's less than a statistical error rate. Time to time you can get results like this:
Closing #153 and same problem for SemanticUI Semantic-Org/Semantic-UI-React#2599
@dcousens , @JedWatson can you take a look?