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

Use SASS for compiling lightbox.css #496

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

helloilya
Copy link

Added sass preprocessor support (issue #495), added buildsass grunt task (sass/autoprefixer).

@jamesstar89
Copy link

Looks good, let's get it in.

@k-funk
Copy link

k-funk commented Jun 2, 2016

Use a var like $lb-image-path: '../images' !default;. It's not uncommon for people to use lots of @imports in their sass/scss, making the relative pathing confusing/wrong. That's why bootstrap-sass does it.

@helloilya
Copy link
Author

Improved styles, fixed a few wrong moments.

@k-funk
Copy link

k-funk commented Jun 20, 2016

LGTM

@lokesh lokesh changed the title Added sass preprocessor Use SASS for compiling lightbox.css Oct 30, 2016
@bahiirwa bahiirwa mentioned this pull request Apr 19, 2017
@bahiirwa
Copy link

+1

@lokesh
Copy link
Owner

lokesh commented Nov 26, 2017

This is an old PR that rewrites the CSS with SASS. I wanted to chime in on why it wasn't merged for anyone who comes across it in the future. For this project I've opted to keep the barrier for usage and outside development very low. So this means no precompilers. At one point I had switched to Coffeescript which caused significant confusion for those unfamiliar with it.

Also, at this point, SASS is making way for PostCSS. And with the minimal amount of CSS in the project, it isn't worth it IMHO.

For features like this that aren't on the roadmap I keep the PRs open so they are visible for others who might be interested in utilizing the code.

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

Successfully merging this pull request may close these issues.

5 participants