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

[fixed] Custom classnames override default styles #100

Merged

Conversation

claydiffrient
Copy link
Contributor

This makes it so if you provide a custom className it will override
all the default styling, relying solely on the style props you pass
in and the className itself.

Prior to this commit, providing a className or overlayClassName
doesn't really have any effect because the style attribute has higher
precedent when applying styles.

This makes it so if you provide a custom className it will override
all the default styling, relying solely on the style props you pass
in and the className itself.

Prior to this commit, providing a className or overlayClassName
doesn't really have any effect because the style attribute has higher
precedent when applying styles.
@jackofseattle
Copy link

👍
Good idea. I was trying to think of a way to do this when I first added in the inline styles. I think this is a very good option.

@claydiffrient
Copy link
Contributor Author

@mzabriskie Your thoughts?

@sergeyk
Copy link

sergeyk commented Dec 9, 2015

👍

@claydiffrient
Copy link
Contributor Author

I've been using this for a bit now... the only caveat that I've come across is that no default styles get applied... so you need to pass in the the default styles that may/may not matter to you into the styles prop when creating the modal.

@grahamb grahamb mentioned this pull request Dec 9, 2015
@chiel
Copy link

chiel commented Dec 16, 2015

Wouldn't it be better to just have a prop, say disableStyles, to disable the default styling, rather than depending on something that's arguably a little bit "magic"? That way, you can still specify custom classnames as well retain default styling.

@claydiffrient
Copy link
Contributor Author

@chielkunkels So this approach doesn't attempt to disable default styles (or at least that's not the end goal). It is to make it so that class names have some value again. The current implementation on master completely relies on the inline styles that are passed in. Because of style precedence however, styles from class names are never applied.

Disabling the default styles just happens to be a byproduct of this approach. It's not the intention of the PR though.

@joefiorini
Copy link

👍 @mzabriskie is this good to merge?

@omarish
Copy link

omarish commented Jan 20, 2016

👍 @mzabriskie this would be huge.

@edbury
Copy link

edbury commented Jan 21, 2016

👍 @mzabriskie would be so pumped on this.

@simpixelated simpixelated mentioned this pull request Feb 1, 2016
@idmontie
Copy link

What is the status of this PR? Is there anything holding it up?

@ChrisZieba
Copy link

Is this project dead, why aren't solid PR's getting merged in?

@claydiffrient claydiffrient merged commit bc58b9c into reactjs:master Mar 24, 2016
@claydiffrient claydiffrient deleted the bugfix/classes-take-precedence branch March 24, 2016 03:10
@aarosil
Copy link

aarosil commented Mar 28, 2016

+1, thank you! Can we get npm version?

@claydiffrient
Copy link
Contributor Author

@aarosil I'm hoping to get an npm version pushed out by the end of the week. I'm hoping to get as many PRs merged in and issues closed before cutting the release. So I'm hoping that the end of the week will give me enough time to take care of most of the current backlog.

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.

10 participants