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

Extract compass #1291

Merged
merged 2 commits into from Jun 11, 2014
Merged

Extract compass #1291

merged 2 commits into from Jun 11, 2014

Conversation

tdreyno
Copy link
Member

@tdreyno tdreyno commented Jun 3, 2014

Really happy with this PR. Started by extracting Compass support into a middleman-compass extension, which required rewriting a bunch of features in Ruby (which we mostly already had) instead of relying on Compass. Lead to nice cleanups all around and heavy usage of the Inline Asset rewriting middleware.

Once this is merged, I'll actually move middleman-compass into its own repo. The question is, do we reference it in the over-gem middleman or require users to add it to their Gemfile?

@Arcovion
Copy link
Contributor

Arcovion commented Jun 3, 2014

If it's moved into it's own gem, why not change compass_config &block to activate :compass &block?
This way you avoid the extra hook and configure it all in one place.

Since it's for v4 I think it's okay to break backwards-compatibility, it's easy to migrate and it'd look more elegant as all of the middleman extensions use that syntax.

@tdreyno
Copy link
Member Author

tdreyno commented Jun 3, 2014

@Arcovion Seems like a good idea, if we decide to break backwards compat.

@tdreyno
Copy link
Member Author

tdreyno commented Jun 5, 2014

Ping @bhollis @karlfreeman

@bhollis
Copy link
Contributor

bhollis commented Jun 6, 2014

I'll have some time to take a look next week - I'm excited about it!

@karlfreeman
Copy link
Contributor

Same as @bhollis. Less is more 👍.

@karlfreeman
Copy link
Contributor

This looks really great!

I'm assuming the logic behind compass_config.environment = :development is to ensure we don't get any production defaults that we're not explicitly setting ourselves?

I would be a fan of requiring users to add it to their Gemfile. Compass is a framework, you wouldn't include Foundation by default, so why Compass. However, not including it by default would need an explanation during the upgrade notes. So can see both sides of it.

👍

@tdreyno
Copy link
Member Author

tdreyno commented Jun 9, 2014

@karlfreeman Correct. That's the way it currently works in stable as well.

Same question for Sprockets too. I'd like to encourage folks to not use either as much, so maybe we pull it out and require a Gemfile change... and see if people get angry. Easily reversible.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 47bcafd on deprecate_compass into * on master*.

@@ -0,0 +1,6 @@
require "middleman-core"

Middleman::Extensions.register(:compass) do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you register this with the auto_activate: :before_configuration option, you won't need the AUTOLOAD_COMPASS stuff.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

@bhollis
Copy link
Contributor

bhollis commented Jun 10, 2014

This is awesome. Good work extracting it! My main comment is the one around using the auto_activate stuff, and also that after this is committed you might want to split it into its own repo.

tdreyno added a commit that referenced this pull request Jun 11, 2014
@tdreyno tdreyno merged commit 4b5c673 into master Jun 11, 2014
@tdreyno tdreyno deleted the deprecate_compass branch June 11, 2014 16:20
@renatocarvalho
Copy link

\o/

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.

None yet

6 participants