i-like-robots/react-tags

Merge classNames instead of overriding the entire object

alexandernst opened this issue · 9 comments

Describe the solution you'd like

I'd like to dynamically add / remove a custom class based on form validation. I tried using classNames, but unless I'm missing something, the object that is being passed to classNames overrides completely the default classes, instead of merging with the default classes and overriding only the ones I have defined.
This behaviour makes the classNames prop very awkward to use, since I must pass all the classes that are used by the widget.

Describe alternatives you've considered

N/A

Additional context

N/A

const classNames = [this.props.classNames.root]

^ A patch could look like this:

- const classNames = [this.props.classNames.root]
+ const classNames = [Object.assign({}, this.props.classNames.root, CLASS_NAMES)] 

and then replace all the this.props.classNames with classNames.

I can create a MR if you're open to this proposal

This is actually the previous behaviour < v6 and I removed it in 205ce74 to avoid using the deprecated componentWillReceiveProps() method. I think I was also considering the performance impact of continuously merging objects (and setting state) unnecessarily because this component doesn't know if any properties within the classNames prop have been updated - much more efficient for implementors to do this when needed!

But why would the code continuously merge objects?
The code needs to merge the props.classNames object with the "defaults" object just once, right before rendering itself. After that, any modification of the classNames object shouldn't reflect any changes on the instance.
If there is a need to modify a class, the developer should re-render the widget (which is basically what happens when a prop is changed the state is changed.

I should have said that I noticed your code example above avoids setting state and therefore any additional update. Due to the way the component is structured now this shouldn't cause any problems. If you want to raise PR for this change then I'd be happy to re-add this behaviour 👍

Okey! I'll submit a MR tomorrow morning

@i-like-robots ready to review ^

@i-like-robots how is the review going? Anything I should change?

is this done? I see it still overrides the classNames on the latest version(6.1.0)