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

Introduce a stylesheet_base generator #767

Merged
merged 1 commit into from
Jun 16, 2016
Merged

Conversation

mike-burns
Copy link
Contributor

This introduces the suspenders:stylesheet_base generator, which adds
our typical foundations for CSS. In this case it is Bourbon, Neat, and
Refills.

Of note, the application.scss must list the imports in a specific
order.

Paired with Justin Moore.

@@ -1,6 +1,7 @@
require 'suspenders/version'
require 'suspenders/generators/app_generator'
require 'suspenders/generators/static_generator'
require 'suspenders/generators/stylesheet_base_generator'

Choose a reason for hiding this comment

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

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@tute
Copy link
Contributor

tute commented May 20, 2016

This looks very happy to me. ❤️ 🚢


module Suspenders
class StylesheetBaseGenerator < Rails::Generators::Base
source_root File.expand_path(
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of keeping the style consistent and re-write this as:

source_root(
  File_expand_path("..", "..", "..", templates),
  File.dirname(__FILE__),
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Everything about this line annoys me. Is there a better way to do this?

@mike-burns
Copy link
Contributor Author

OK so the status of this seems to be:

  • Everyone hates the hideous source_root line.
  • Concern over users who have an existing application.css.

@tute @teoljungberg Are these things that should be fixed in this PR?

@tute
Copy link
Contributor

tute commented Jun 10, 2016

Everyone hates the hideous source_root line.

If we know of a better way we improve it yes, but if not, it shouldn't hold off this PR I think.

Concern over users who have an existing application.css.

Around this your previous last comment said:

Thinking about this more, I'm tempted to issue no warning and not touch anything except app/assets/stylesheets/application.scss.

What do you think -- is that setting someone up for hours of annoying debugging?

I'm ok with that workaround. But there is a bug right now: application.css is not removed any longer:

test1 master % tree app/assets/stylesheets
app/assets/stylesheets
├── application.css
├── application.scss
└── base
    ├── _base.scss
    ├── ...

@teoljungberg
Copy link
Contributor

Everyone hates the hideous source_root line.

If we know of a better way we improve it yes, but if not, it shouldn't hold off this PR I think.

I agree with @tute that that can be left for a future PR.

@mike-burns mike-burns force-pushed the gen-stylesheet_base branch from 1e2436d to b450d9b Compare June 15, 2016 11:16
@mike-burns
Copy link
Contributor Author

But there is a bug right now: application.css is not removed any longer

Aha! I had missed that. Obvious now. Thanks, fixed.

Now the build is failing because of changes to the gem ecosystem. We should fix master before I merge.

@mcmire
Copy link
Contributor

mcmire commented Jun 15, 2016

I just saw this and this seems like a great addition. Thanks @mike-burns.

@mike-burns mike-burns force-pushed the gen-stylesheet_base branch from b450d9b to 5c611ff Compare June 16, 2016 11:21
This introduces the `suspenders:stylesheet_base` generator, which adds
our typical foundations for CSS. In this case it is Bourbon, Neat, and
Refills.

Of note, the `application.scss` must list the imports in a specific
order, and this removes the `application.css`.
@mike-burns mike-burns force-pushed the gen-stylesheet_base branch from 5c611ff to 3625422 Compare June 16, 2016 12:22
@mike-burns mike-burns merged commit 3625422 into master Jun 16, 2016
@mike-burns mike-burns deleted the gen-stylesheet_base branch June 16, 2016 12:23
@rikkipitt
Copy link
Contributor

I haven't had chance to properly delve into this new generator, but are you guys getting the following when bundling? It's because the refills gem is already in the development/test block in the Gemfile while the new generator appends it again to the bottom of the file...

Your Gemfile lists the gem refills (>= 0) more than once.
You should probably keep only one of them.
While it's not a problem now, it could cause errors if you change the version of just one of them later.

@mike-burns
Copy link
Contributor Author

@rikkipitt I'm not seeing that. Attaching a sample Gemfile that I see. I did this to get it:

mkdir tmp && cd tmp
../suspenders/bin/suspenders sample

Gemfile.txt

@rikkipitt
Copy link
Contributor

@mike-burns Looks like someone else has found this to be the case too in #786

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