Skip to content

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

Merged
TheSharpieOne merged 1 commit intoreactstrap:masterfrom
ajs139:css_modules_fixes
Jul 27, 2017
Merged

fix(DropdownToggle, NavbarToggler, Tooltip): Pass through cssModule to child components#483
TheSharpieOne merged 1 commit intoreactstrap:masterfrom
ajs139:css_modules_fixes

Conversation

@ajs139
Copy link
Copy Markdown
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
Comment thread src/ButtonGroup.js Outdated

return (
<Tag {...attributes} className={classes} />
<Tag {...attributes} cssModule={cssModule} className={classes} />
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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/css-loader#520 is resolved.

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

Copy link
Copy Markdown
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
Copy Markdown

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.

3 participants