Skip to content

Added new prop to customise css class#18

Open
hugofelp wants to merge 3 commits intotakempf:masterfrom
hugofelp:master
Open

Added new prop to customise css class#18
hugofelp wants to merge 3 commits intotakempf:masterfrom
hugofelp:master

Conversation

@hugofelp
Copy link

First of all, thanks for taking the time to share this package!

This is a very simple addition, in order to allow customisation of active state class.

My use case for this addition is to be able to use existing css (bootstrap lib for instance), instead os adding Dropdown.css, taking advantage of js functionality and nothing else. All elements already accept additional classes via className, so this was the only thing preventing a full customisation.

I have also updated the readme file for clarity.
Thanks

@takempf
Copy link
Owner

takempf commented Aug 14, 2016

Thanks for the pull req!

I've been thinking about this for a while now, and I believe we should use a more generalized approach. I sat down and experimented for a bit, and I think this is probably the best way to move forward:

  • Create an object that contains all of the default classes used in this component (something like DEFAULT_CLASSES = { dropdown: 'dropdown', 'dropdown-active': 'dropdown--active' ... })
  • Add a new prop, customClassNames, that is an object with the same keys as DEFAULT_CLASSES
  • Merge DEFAULT_CLASSES and this.props.customClassNames at some point
  • Use the combined classes object to supply classes the component and subcomponents
  • Add tests for the customClassNames prop

What's your opinion on this approach?

@hugofelp
Copy link
Author

To be honest, I prefer the way it currently is. Mainly because it is closer to regular jsx, where I apply classes to the elements directly. If I want to apply a class to DropdownTrigger, I think it makes more sense to use the className prop of that element than passing it as an object to its parent.

Although, your approach would be better prepared for future additions, for example, disabled state (I can't think of anything else really). Another point is that I would be able to prevent the current default classes from being applied to the elements. In my case, they do no harm, because they don't exist in my css, but I see they could.

Based on these thoughts, my preferred approach would be to keep current props the way they are, and just not apply default classes if className has been provided to a given element.

Either way, it will be a step forward from current state, so I am happy to implement any of these approaches :)

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 this pull request may close these issues.

3 participants