Problem/Motivation

This is a great theme and support for Sass is one of its major benefits, but the current Sass implementation feels somewhat confusing and difficult to use. The near direct copy of CSS from the HTML5 Boilerplate project means that many elements are unnecessarily defined multiple times in different places and Sass variables are not always respected. Mixins and partials are underutilized, neglecting much of the power of Sass. There are also significant inconsistencies in the code syntax across files.

Proposed resolution

Change _variables.scss to _base.scss. This should include the Sass variables as well as imports that need to be included in all .scss files, like compass and the custom _mixins.scss file.

Turn the current default.scss, layout.scss, mobile.scss and print.scss files into partials that are assembled in style.scss, creating one base style sheet for inclusion on all pages. Boilerplate.info will need to be changed to reflect this.

Get rid of _iphone.scss. it serves no purpose and the same code is already in mobile.scss.

Combine the styles in print.scss with the print styles in default.scss and put them in the new _print.scss. All of these styles should be contained in a single "@media print" statement, to get the benefits discussed at h5bp.com/r.

The final file configuration should be something like this:

  • _base.scss => define sass variables and partials to be included in all .scss files
  • _custom.scss => define custom styles for page elements
  • _defaults.scss => establishes general rules and browser resets
  • _layout.scss => define the layout (positioning) of the theme's major elements
  • _mixins.scss => define custom sass functions and mixins for use in other .scss files
  • _mobile.scss => define the way the theme addapts on mobile devices
  • _print.scss => define the way the theme look like when printed
  • style.scss => assembles all partials into a base css to be loaded on every page

Sass syntax and comments should also be cleaned up across all files.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jstoller’s picture

Status: Active » Needs review
FileSize
114.36 KB

This patch cleans up and reorganizes all Sass files, as discussed above. In doing so, it negates issues #1447562: SCSS import syntax and #1451478: Add a config.rb file for Compass/Sass support. It also improves the documentation some.

I left the ie6 and ie7 files in place, pending issue #1449636: Does Boilerplate use the ie stylesheets?, but I expect that they could be removed as well.

I resisted the temptation to make any changes of substance to the structure or styling of the theme, so it should render exactly the same as before.

basicmagic.net’s picture

@jstoller

good ideas... not sure how the boilerplate people will consider them for future development-
but i applied your patch and am very happy with the results.

everything is working good and looking great, so far.

i made a few tweaks in the templates and style sheets too-
and will have 2-3 new sites live with custom boilerplate based themes, within the next week or two-
this theme is the greatest!

fyi, see my post here-
http://drupal.org/node/1458946

jstoller’s picture

@basicmagic
Glad you like the patch. If you feel you've thoroughly reviewed it and haven't found any bugs, please consider changing the status of this issue to RTBC.

jstoller’s picture

Priority: Normal » Major

I've marked #1447562: SCSS import syntax and #1451478: Add a config.rb file for Compass/Sass support as duplicates of this issue, pending maintainer review, as this patch supersedes both of them.

I'm also bumping the priority to major, due to the scope of this refactoring. My own use of this theme is dependent on it having solid Sass support and I'm waiting on this being committed before making any further contributions.

jstoller’s picture

@danlinn:
Just wondering if you've had a chance to review this yet and if it meshes with your plans for Boilerplate. Perhaps we can connect at DrupalCon and discuss it.

adrinux’s picture

Status: Needs review » Reviewed & tested by the community

For the changes in general, the new layout seems more logical to me. Also as a sass newbie I find the fact that it matches the compass best practice documentation (see http://compass-style.org/help/tutorials/best_practices/ ) more closely, helpful. Patch applies without error too.

Marking this reviewed. At this point it needs an executive decision I think.

(edited to remove some irrelevant comments)

adrinux’s picture

Status: Reviewed & tested by the community » Needs work

Spotted one issue. patch deletes mobile.scss but still imports it in style.scss.

@jstoller Did you make a mobile.scss partial?

jstoller’s picture

@adrinux:
You should find a _mobile.scss partial in the the sass directory. Is it not there when you apply the patch?

jstoller’s picture

Status: Needs work » Needs review
FileSize
118.37 KB

I've rerolled the patch. It now includes the ie6 and ie7 styles as partials, instead of the individual style sheets. I also refactored those styles a bit and fixed a couple bugs. They now use the lt-ie8 and lt-ie7 classes, so they will only take effect when needed.

jstoller’s picture

FileSize
118.38 KB

Re-roll to update README file.

danlinn’s picture

Alright, Jstoller. I'm working on the next release now and will use this patch.

jstoller’s picture

Status: Needs review » Closed (fixed)

Committed and released.