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

fix(DropdownToggle, NavbarToggler, Tooltip): Pass through cssModule to child components #483

Merged
merged 1 commit into from
Jul 27, 2017

Conversation

ajs139
Copy link
Contributor

@ajs139 ajs139 commented Jun 28, 2017

Trying to use CSS Modules proved problematic for a few of the components. These changes pass through the CSS Module to the child component when appropriate.

@ajs139 ajs139 changed the title fix: Pass through cssModule to child components fix(ButtonGroup, DropdownToggle, NavbarToggler, Tooltip): Pass through cssModule to child components Jun 28, 2017
@@ -35,7 +35,7 @@ const ButtonGroup = (props) => {
), cssModule);

return (
<Tag {...attributes} className={classes} />
<Tag {...attributes} cssModule={cssModule} className={classes} />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here the default Tag is div, in which case the cssModules prop would not want to be passed down

@TheSharpieOne
Copy link
Member

I like what you are trying to do here, and you definitely exposed a pretty large issue with the way tag works with cssModules but I don't know if this is the best way to solve it.
We do not want cssModules being passed down when the Tag is not a reactstrap component, but we don't know what Tag is when it is a component. Several places you can pass one reactstrap component to another as a tag and it would make sense, and in those cases, the component passed as the tag would not receive cssModule.
The larger change would be to change the way css modules integrate and using something like a wrapping provider which exposes the modules in the context and have each of the components use the context to get the modules... but IDK if that is wanted as this project aims to be simple and not use context except for where absolutely necessary.

@ajs139 ajs139 changed the title fix(ButtonGroup, DropdownToggle, NavbarToggler, Tooltip): Pass through cssModule to child components fix(DropdownToggle, NavbarToggler, Tooltip): Pass through cssModule to child components Jun 28, 2017
@ajs139
Copy link
Contributor Author

ajs139 commented Jun 28, 2017

Thanks for the feedback, I see exactly what you mean. I've removed the changes to ButtonGroup. The other three changes seem, less problematic, is that right?

I'll give more thought to the ButtonGroup and if I come up with anything I'll let you know.

@ajs139
Copy link
Contributor Author

ajs139 commented Jun 29, 2017

@TheSharpieOne - on reflection, I'm not sure changing ButtonGroup was the right plan anyway.

The idea was to be able to give ButtonGroup a CSS Module that would style its child components, but I think this will really be easy when webpack-contrib/css-loader#520 is resolved.

Please let me know if you need anything else from me.

Copy link
Member

@TheSharpieOne TheSharpieOne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@TheSharpieOne TheSharpieOne merged commit 12270d0 into reactstrap:master Jul 27, 2017
@kuzvac
Copy link

kuzvac commented Aug 18, 2017

Hi guys, thank you all for this fix. @TheSharpieOne when we can expect this fix in npm release? :)

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.

None yet

3 participants