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

Allow multiple content classes #95

Closed
wants to merge 3 commits into from
Closed

Allow multiple content classes #95

wants to merge 3 commits into from

Conversation

mstenquist
Copy link

I noticed an issue when vex.baseClassNames.content was set to a string containing multiple classes.

For example...

vex.baseClassNames.content = 'vex-content' (works fine)

vs.

vex.baseClassNames.content = 'vex-content vex-content-auto hs-modal' (breaks)

The culprit was $(@).parents(".#{vex.baseClassNames.content}") which tries to form a selector '.vex-content vex-content-auto hs-modal' Needless to say, this doesn't work.

The solution was to add a utility method getBaseClassesAsSelector to vex. It's job is to simply split/join and return our properly formatted selector .vex-content.vex-content-auto.hs-modal

@timmfin @zackbloom @gusvargas @b-ash

@b-ash
Copy link

b-ash commented Jan 5, 2015

@adamschwartz
Copy link
Contributor

Hey @mstenquist, thanks so much for this!

I think this is a great suggestion, but your particular implementation strikes me as a little strange. Rather than have the method getBaseClassesAsSelector return the content selector specifically, I think it would make more sense to pass in base class and have it return the selector. That way this could be used for more classes than just vex.baseClassNames.content.

Also, I think the name would make more sense as getSelectorFromBaseClass. So altogether it would look something like this:

vex.getSelectorFromBaseClass(vex.baseClassNames.content)

@mstenquist
Copy link
Author

@adamschwartz Ooo.. I like that. Thanks for the feedback.

@mstenquist mstenquist closed this Jan 5, 2015
@mstenquist mstenquist deleted the allow_multiple_content_classes branch January 5, 2015 20:13
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