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

make sprite vars default to be able to preset outside of chosen #1736

Merged
merged 1 commit into from
Jan 22, 2014
Merged

make sprite vars default to be able to preset outside of chosen #1736

merged 1 commit into from
Jan 22, 2014

Conversation

benib
Copy link

@benib benib commented Jan 21, 2014

This sets $chosen-sprite and $chosen-sprite-retina to default. It is then possible to define these variables before importing the chosen.scss file.

This gives you more flexibility to use your own folder structure since compass only knows about one images_dir per project and computes the image-url relative to this config setting.

@pfiller
Copy link
Contributor

pfiller commented Jan 21, 2014

Thanks, @benib. I'm going to defer to the opinions of @mlettini and @starzonmyarmz on this one.

@starzonmyarmz
Copy link
Member

Interesting, I was not aware of Sass !default. I could see the benefit's of adding this. Especially if you want to use Chosen in a Rails app or something. @mlettini how do you feel about this? Quick reading: http://robots.thoughtbot.com/sass-default

@stof
Copy link
Collaborator

stof commented Jan 21, 2014

looks like a good idea

@mlettini
Copy link

I've never used it before, but it sounds really useful. Especially for this use-case as a 3rd-party css file. I'm game.

Then again, if someone is customizing the sprite, it might be safe to assume they probably are also customizing other css styles too, and would have to put the overrides below the chosen css anyway. I don't know.

@starzonmyarmz
Copy link
Member

@mlettini I was thinking that too, but I think that can happen sometime down the road. Anyway... +1 for this PR

@benib
Copy link
Author

benib commented Jan 21, 2014

@mlettini This is not only useful when you want to customize the sprite, but also when you use compass for your own project and want to @import the chosen.scss in your own sass files. Compass then resolves the path of $chosen-sprite against the images_dir of your own config.rb, not the one from choosen, so you need to set the path to the choosen sprite relative to your own images_dir.

@mlettini
Copy link

Ah right, that makes a lot of sense.

Does Chosen repo get a lot of questions about where to put the sprite images in their projects? Maybe we could have a comment in the css file, or at least on the project page, denoting that you can put them anywhere in your images_dir and are allowed to define the variables.

@benib
Copy link
Author

benib commented Jan 22, 2014

@mlettini should I add a comment to the scss file about the possibility to change these variables, or is there something else blocking this PR?

I think it would be cool to have the scss more customizable by using more variables. But this could be another PR.

@mlettini
Copy link

My comment was certainly non-blocking to this PR. Just bringing up the idea to add some comments to help out any users that might be confused about it. Probably best for another time.

@tjschuck
Copy link
Member

We should probably add a section to the actual docs about customizing Chosen's appearance. This would fit well there. Non-blocking on this, though -- I'll open another issue about that for the future.

@mlettini @starzonmyarmz This have a +1 to merge then? I'll merge and update the release notes if you give your go-ahead.

@mlettini
Copy link

+1

tjschuck added a commit that referenced this pull request Jan 22, 2014
make sprite vars default to be able to preset outside of chosen
@tjschuck tjschuck merged commit a0f3e58 into harvesthq:master Jan 22, 2014
@tjschuck
Copy link
Member

Thanks @benib!

@benib benib deleted the sprite-vars-default branch January 22, 2014 18:01
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.

6 participants