Skip to content
This repository was archived by the owner on May 29, 2019. It is now read-only.

feat(modal): Add backdropClass option, similar to windowClass option #1862

Closed
wants to merge 1 commit into from
Closed

feat(modal): Add backdropClass option, similar to windowClass option #1862

wants to merge 1 commit into from

Conversation

jsanders
Copy link
Contributor

Fixes #1179

Really, identical to windowClass, but for backdrop.

@fxck: I used a different option name because we have the use case of needing both at the same time and concatenating classes like you suggest doesn't seem like the right answer when what we really want is two different classes on two different elements.

Really, identical to windowClass, but for backdrop.

Fixes #1179
@sonarxavier
Copy link

Any ETA on when this will be merge?

@jsanders
Copy link
Contributor Author

jsanders commented Mar 6, 2014

@sonarxavier: I haven't heard anything about it from anybody involved with the project. Maybe needs a few people to +1 it to get some attention.

@Foxandxss
Copy link
Contributor

Just 3 days :P. We have more responsibilities apart from this lib heh.

That aside, I like the PR, minimum change, tested and doc updated. I am not so sure of the use case, but the final decision is from @pkozlowski-opensource :)

@jsanders
Copy link
Contributor Author

jsanders commented Mar 9, 2014

@Foxandxss Whoops, I really didn't mean my comment to come off as a "geez, what's taking so long" at all :)

Very specifically, our use case is that we've changed the styling of one of our modals in such a way that the style of the backdrop makes it look bad. We can change the style of the backdrop, but that will apply to the backdrop of all the modals, rather than just the one that is problematic. So we'd just like a way to independently style backdrops like we can currently independently style the modals themselves.

Thanks for taking a look!

@Foxandxss
Copy link
Contributor

That makes sense and it is an small change, so that is 👍 for me, still @pkozlowski-opensource has the word.

@nolazybits
Copy link

@jsanders sweet. Hopefully it will be integrated soon :)

@mwilc0x
Copy link

mwilc0x commented Mar 10, 2014

+1 Just ran into a situation on a project where this would very helpful. Awesome to see that a solution has been proposed. Would be nice to see this make it in.

@fxck
Copy link

fxck commented Mar 10, 2014

@jsanders mind explaining that use case where you need this, and it's not possible doing the way I suggested?

@jsanders
Copy link
Contributor Author

@Foxandxss Excellent, thanks!

@fxck To make sure I understand your suggestion correctly - you want to re-use the existing windowClass option for both the modal itself and the backdrop, so if eg. we set windowClass to custom we would style the modal using .custom.modal and the backdrop using .custom.modal-backdrop. Correct me if that's not the right interpretation.

I think that would absolutely work, but that separating them into two options just makes it more obvious how they are meant to be used. If others who are interested in this think your way is simpler, I'm not at all against it. This is just the way that made the most sense to me.

@fxck
Copy link

fxck commented Mar 11, 2014

Yeah, that's absolutely fine, I just wanted to know whether there's actually a case where you'd need backdropClass, because it wouldn't be possible to do using windowClass only.

@jsanders
Copy link
Contributor Author

Ah, without the change in this pull request, we can't style the backdrop using windowClass, because that is only applied to the modal itself. So some change is needed to be able to do that. The change could be to make windowClass apply to backdrops like you suggest, or it could be to add a new option for the class of the backdrop, like I've done here. I think either one would work, but I like having a separate option more.

@jared2501
Copy link

+1
Also, after a brief read I think a separate windowClass and backdropClass is a good idea. This gives the user flexibility to reuse a backdropClass between modals, but have a different windowClass between them.

@pkozlowski-opensource
Copy link
Member

Landed, thnx @jsanders !

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

custom class in modals backdrop
8 participants