I learned about this module in a training and think it is fun. I noticed that there is a check to make sure that the respond.js file is not being included by an @import so it nags the user to aggregate the javascript.

That is great, but if the user has the conditional style sheet module enabled they get this kind of included javascript:

<script type="text/javascript" src="/sites/all/libraries/respondjs/respond.min.js?m0oy5u"></script>

I think that is *good to go* so I added a patch to check if the user has conditional styles installed and not nag them in that case.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rupl’s picture

It's not dependent on the respond.js <script> tag, which will always be included that way. The crucial requirement is having stylesheet <link> tags that should be used in lieu of @imported styles. Here's the passage on the project page currently:

Respond.js only works on <link> tags; it will not work on @import stylesheet calls. Drupal 7 uses @import when CSS aggregation is disabled, but uses <link> when CSS aggregation is enabled. The module will complain with an admin error until you enable aggregation.

Reading it again, I think I'm wording the documentation poorly. Maybe it should read:

Respond.js only works when Drupal's stylesheets are included using <link> tags; ...

populist’s picture

Title: Support Conditional Styles Module » Moving the CSS aggregation nag to hook_requirements()
Category: bug » feature
FileSize
1.56 KB

Thanks for the clarification here.

Now allow me to pivot the issue to discuss the hook_init() provided user nag. How would you feel about moving that into hook requirements instead? It seems to make more sense in there (like the other two hook_requirement considerations) than on every admin screen. I made a patch.

rupl’s picture

Assigned: Unassigned » rupl

Yeah I can see doing that. This tiny bit of config has accounted for probably 80% of support requests, so when I added the warning, I went for full-annoyance mode :) Patch looks good on visual inspection, will review/commit during Denver. Thanks, Matt!

populist’s picture

Maybe we can make the "nag" the default for first time users, but can be easily hide "Dont Nag Me Anymore!" and then we just use hook_requirements() for the real check?

rupl’s picture

Ah yes, like the Advanced Help message in Views, that's a good idea.

rupl’s picture

Status: Needs review » Fixed

The move to hook_requirements() was committed to dev and will be released as part of 1.1 very shortly. Thanks for the patch!

I opened up #1487030: Allow admins to disable the severity of CSS aggregation warning as a follow up to your other suggestion.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Wrapped HTML in so it displays in the summary.