woocommerce / storefront

Official theme for WooCommerce
https://wordpress.org/themes/storefront/
970 stars 471 forks source link

Suggestion: Use $vpsacing and more unit spacing variables #431

Closed patrick-wc closed 8 years ago

patrick-wc commented 8 years ago

Hi,

I've just started use storefront in a project using galleria child theme and I noticed a lot of usage of 1.618em in many of the sass partial stylesheets. There is already a variable called $vspacing which is also used in some partial stylesheets, so I'm confused as to why that isn't used instead of repeating 1.618em all over the shop ;) I assume its some kind of legacy from when sass wasn't used, but I thought it would be a good idea to get things more organised spacing units wise.

My first thought was to do a quick search replace and pull request, but I was wondering how the other em values have been calculated, e.g. just from looking at _base.scss there is 4.236em, 2.618em, 1.387em and more. I think it would be worthwhile to use some sort of mixin or a set of spacing variables - e.g. $vspacing--half $vspacing--double or whatever so when another developer comes to look at the SCSS for this project they can quickly see the thinking behind it. A quick google tells me its something to do with vertical rhythm and the golden ratio... perhaps someone more involved in storefront could explain the thinking behind the values, then if you don't mind I could add some variables or mixins and do a PR or however you think it's best for me to contribute.

Cheers :)

jameskoster commented 8 years ago

Yup, this is something we should address for sure :)

The numbers are all based on a modular scale.

jameskoster commented 8 years ago

@zerodburn were you planning to submit a PR for this? No worries if not, just don't want to start working on something you might already be looking at xD