Increase spacing scale #376

gladwearefriends · 2017-10-23T21:34:55Z

TLDR

With primer-marketing we now have an extended spacing scale to use for y-axis spacing

$spacer-7:  48px
$spacer-8:  64px
$spacer-9:  80px
$spacer-10: 96px
$spacer-11: 112px
$spacer-12: 128px

and corresponding utility spacers for
mt-, mb-, my-, pt-, pb-, py-

e.g. mt-7, mb-9, py-11

Testing on github/github here: https://github.com/github/github/pull/81032
Deployed to review lab here: https://testing-primer.review-lab.github.com/
To test on review-lab, slap some of those new classes onto some elements!

This has also been tested in storybook.

Hi friends!
Wanted to get a PR open to see if this works and also so we could discuss about exactly which increments to increase our spacing scale with. Previous conversation at https://github.com/github/design-systems/issues/378 where we established we needed this on marketing sites.

Some ideas that have been proposed.
After 40px,

jump by 16px
jump by 40px
jump by powers of 2 (64, 128, 256)

In this PR i went ahead and jumped it by 16px. Only adding spacer utilities for y-directional spacing. i.e. mt-7, pb-7, my-7

jump by 16px (56, 72, 88, 104, 120)
- PROS: addresses the various spacers we have now with the .featurette (50, 60, 80) and .jumbotron (60, 100, 120) classes for github marketing pages.
- CONS: Bloats up our margin and padding utilities a bit. But ive gone ahead and put this into only our primer-marketing-support and primer-marketing-utilities modules Not sure if how i did this was the most elegant though. We need to run the new $spacers variable through the margin & padding utility mixins again but not the most DRY way of doing that.
jump by 40px
- PROS: addresses most of our needs for spacers without bloating our utilities
- CONS: doesnt address all of our current spacers that we use. jumping from 40 to 80, skips out on the 50/60px range we currently achieve with the .featurette and .jumbotron classes
jump by powers of 2 (64, 128, 256)
- PROS: gets big quickly, no bloat
- CONS: gets big quickly 🙃 not as useful in some cases and arguable if we really need a 256 spacer

/cc @primer/ds-core @sophshep

sophshep

Thanks for starting this @gladwearefriends!!

Left some notes and questions inline.

sophshep · 2017-10-25T01:58:31Z

modules/primer-marketing-support/lib/layout.scss

+) !default;
+
+// Adds aliases
+$spacer-7:  nth($spacers, 8) !default;  // 56px


We shouldn't redefine the whole map again, it will likely result in a lot of duplicate and unnecessary compiled CSS.

Instead, to keep things simple and readable, we can just define the variables manually (like: $spacer-7: $spacer * 7). This can be added onto the existing variable file: https://github.com/primer/primer-css/blob/master/modules/primer-marketing-support/lib/variables.scss

sophshep · 2017-10-25T02:07:20Z

modules/primer-marketing-utilities/lib/margin.scss

@@ -0,0 +1,80 @@
+// Margin spacer utilities
+// stylelint-disable block-opening-brace-space-before, declaration-colon-space-before, primer/selector-no-utility, comment-empty-line-before
+@for $i from 1 through length($spacers) {


Likewise here, we don't need this full loop since it will generate CSS for allll the spacer variables instead of just the biggos you're adding.

Two options:

Specify which values you want to run the loop to limit it to just the new marketing spacers

Ditch the fancy Sass loop all together and write it manually

i'm gonna specify the new variables so the loop only goes through the new marketing spacers

sophshep · 2017-10-25T02:10:10Z

modules/primer-marketing-utilities/lib/margin.scss

+  /* Set a #{$size} margin on the top */
+  .mt-#{$scale} { margin-top   : #{$size} !important; }
+  /* Set a #{$size} margin on the right */
+  .mr-#{$scale} { margin-right : #{$size} !important; }


Do we need margin right and left for these? I can't think of any situations where I've wanted bigger values for these than what we've got. Currently, I feel like we just need the vertical values (to allow us to refactor jumbotron and featurette custom css). Taking these out will reduce a lot of final CSS.

(I'd be curious to hear from others on @primer/design-systems on if they agree)

im gonna remove them unless others think otherwise! @primer/design-systems

sophshep · 2017-10-25T02:10:33Z

modules/primer-marketing-utilities/lib/margin.scss

+  /* Set a #{$size} margin on the left */
+  .ml-#{$scale} { margin-left  : #{$size} !important; }
+  /* Set a negative #{$size} margin on top */
+  .mt-n#{$scale} { margin-top   : -#{$size} !important; }


I don't think we need biggo negative margins.

sophshep · 2017-10-25T02:13:28Z

modules/primer-marketing-utilities/lib/padding.scss

+// Padding spacer utilities for marketing
+// stylelint-disable block-opening-brace-space-before, declaration-colon-space-before
+// stylelint-disable comment-empty-line-before
+@for $i from 1 through length($spacers) {


All the notes I left for margin apply to padding too.

sophshep

These changes all look good to me. Can you make sure to write docs for the spacers?

sophshep · 2017-11-02T16:40:39Z

modules/primer-marketing-utilities/lib/margin.scss

+// Margin spacer utilities for marketing
+// Utilities only added for y-direction margin (i.e. top & bottom)
+// stylelint-disable block-opening-brace-space-before, declaration-colon-space-before, primer/selector-no-utility, comment-empty-line-before
+@for $i from 1 through length($marketingSpacers) {


gladwearefriends · 2017-11-02T20:32:46Z

modules/primer-marketing-support/lib/variables.scss

@@ -1,3 +1,13 @@
 // Type
 $alt-body-font: Roboto, -apple-system, BlinkMacSystemFont, "Helvetica Neue", "Segoe UI", "Oxygen", "Ubuntu", "Cantarell", "Open Sans", sans-serif;
 $alt-mono-font: $mono-font;


hmm should I move these variables to the primer-marketing-type module? @sophshep @broccolini

I would keep them there, it allows them to be used in any modules for marketing.

broccolini

Thanks for working on this @gladwearefriends 💖. Requesting some documentation changes.

broccolini · 2017-11-04T02:25:34Z

modules/primer-marketing-support/lib/variables.scss

@@ -1,3 +1,13 @@
 // Type
 $alt-body-font: Roboto, -apple-system, BlinkMacSystemFont, "Helvetica Neue", "Segoe UI", "Oxygen", "Ubuntu", "Cantarell", "Open Sans", sans-serif;
 $alt-mono-font: $mono-font;


I would keep them there, it allows them to be used in any modules for marketing.

broccolini · 2017-11-04T02:40:28Z

modules/primer-marketing-utilities/README.md

@@ -33,7 +33,7 @@ $ npm run build

 ## Documentation

-
+You can read more about utilities in the [docs folder](./docs/).


broccolini · 2017-11-04T03:24:15Z

modules/primer-marketing-utilities/docs/margin.md

@@ -0,0 +1,38 @@
+---
+title: Margin


The titles in the docs are also used in the navigation for the style guide so we need to prefix marketing styles with marketing when there is a duplicate styles like margin.

broccolini · 2017-11-04T04:13:27Z

modules/primer-marketing-support/README.md

@@ -38,6 +27,24 @@ The source files included are written in [Sass][sass] (`scss`) You can simply po

 You can also import specific portions of the module by importing those partials from the `/lib/` folder. _Make sure you import any requirements along with the modules._

+## Documentation


When documentation is in a readme it needs to be wrapped in these tags so that we can pull only the documentation part, and not the rest of the readme, into the styleguide.

 ## Documentation goes here 

broccolini · 2017-11-04T05:15:16Z

modules/primer-marketing-support/README.md

+
+### Extended spacing scale
+This module includes extra variables that extend the Primer spacing scale for marketing site needs.
+Starting from where the `primer-core` spacing scale ends, we are stepping up by 16px increments...


Were you intending Starting from... to be on the same line or a new paragraph? You'll need an extra line space if you want it to be on a new line. Small formatting thing, can you put the 16px in code tag, like this 16px 😁 .

We don't usually say "we", or shouldn't do once we update the voice and tone of the documentation, use back ticks on any units, and rather than the ... use a colon at the end of the sentence. Also, I think included the spacer units helps.

Starting where the primer-core spacing scale ends at spacer 6, the marketing scale steps in increments of 16px from spacer 7 up to 11.

broccolini · 2017-11-04T16:41:35Z

modules/primer-marketing-utilities/docs/margin.md

+
+Use y-directional utilities to apply margin to the top, bottom or Y axis of an element. Directional utilities can change or override default margins, and can be used with a spacing scale of 7-11.
+
+Note: The marketing spacing scale extends the core 0-6 scale. See [primer-marketing-support](https://github.com/primer/primer-css/tree/master/modules/primer-marketing-support) for the extended marketing spacing scale.


You can remove this note if you update the intro para as per comment above.

broccolini · 2017-11-04T16:44:18Z

modules/primer-marketing-utilities/docs/margin.md

+
+## Responsive y-directional margins
+
+All y-directional margin utilities can be adjusted per [breakpoint](/styleguide/css/modules/grid#breakpoints) using the following formula: `m[y-direction]-[breakpoint]-[spacer]`. Each responsive style is applied to the specified breakpoint and up.


Suggest remove y-directional with marketing:

All marketing margin utilities can be adjusted per...

broccolini · 2017-11-04T16:45:30Z

modules/primer-marketing-utilities/docs/margin.md

+</div>
+```
+
+## Responsive y-directional margins


Suggest changing heading to: Responsive y-axis margin utilities

broccolini · 2017-11-04T16:46:12Z

modules/primer-marketing-utilities/docs/padding.md

+status_issue: https://github.com/github/design-systems/issues/378
+---
+
+Padding spacing utilities can be applied to elements to achieve bigger y-directional spaces beyond the `primer-core` spacing scale.


Could you make the same changes to padding based on the changes I suggested for margins?

broccolini · 2017-11-04T16:53:57Z

modules/primer-marketing-utilities/docs/margin.md

+
+## Y-Directional margin spacing
+
+Use y-directional utilities to apply margin to the top, bottom or Y axis of an element. Directional utilities can change or override default margins, and can be used with a spacing scale of 7-11.


I notice you're using "y-directional" quite a lot through the docs. It doesn't really roll off the tongue very well and I don't think it's a commonly used term 😬. In this instance I think you can get away from using by changing the sentence to the following:

Use marketing margin utilities to apply margin to top, bottom, or y-axis of an element.

I know it says "margin" twice but kinda difficult to get away from that here.

Haha great point @broccolini ! I think i pulled it from our usage of "directional margins" and ran with it... agreed, axis is a much better term. I'll update it!

broccolini · 2017-11-04T17:30:07Z

😬 😞 🙈 Going to pre-face this comment with a huge sorry to @gladwearefriends for not thinking of this earlier, but while I was reviewing the pr, something dawned on me and I think best to share now rather than ignore it. I had looked at the initial values and thought "they seem fine, if that's what works for marketing then cool", but then something started to bother me...

I kept looking at 7 to 11 thinking this feels like an odd range, why don't we have 6 like core? And then I thought about how it's bothered me for a while that we start with a half step of 4px in core, and so I decided to map out the spacing scale to see how it would affect things if we stepped in increments of 8px throughout core, and how that would then impact marketing.

This led me to seeing what happens if we started the marketing scale on 48 (because if we re-mapped core to 8px steps from 1-6 we'd end on 48px instead of 40px. When I stepped the marketing scale from 48px and up, the scale lands on a lot more values that map to the rest of the scales in core and marketing. It lands on a lot more powers of 2 which essentially make for more composable numbers, i.e. more numbers that can be divided by or added up to make other numbers in our scales.

This is what I aimed for with our other scales because design is often about horizontally and vertically lining a bunch of small and medium sized elements with other larger elements etc. Here's the re-mapped scale starting from 48px which I quickly checked against the type and spacing scales:

Spacer	Value	Is it highly composable?
7	48	Medium: maps to type scale 00, 2, & 6; and spacer 10
8	64	High: power of 2; maps to spacer 1,2,3, & 5; type scale 4 & 1
9	80	Medium: double spacer 5, & type scale 3 and 0
10	96	Medium: maps to type scale 00, 2, & 6; and spacer 7
11	112	Low: maps to type scale 5 (body font size)
12	128	High: power of 2; maps to spacer 1,2,3, & 5; type scale 4 & 1

I think the marketing scale could have 6 steps like the core scale, that feels more natural and what people would expect. It also means starting the scale on a half step just like core. This is a fault of our core scale starting on a half step, we could have separated the 4px value out and called it "nudge" or "half-step" or something, and then had a more logical scale that always stepped in increments of 8px.

I know the timing of this is not good, it's very late in the process. We're working up to a release next week, and @gladwearefriends ❤️ ❤️ ❤️ has done a ton of work on adding the Sass for the new scale, writing docs, and updating the GitHub codebase. However, after having shipped a not-so-great spacing scale in the past and then having to update it (ht Rohan and Mu-an), it's a painful process and could be worth the effort now to ship this with a better more composable scale—assuming it actually works for marketing too.

@sophshep @gladwearefriends @jonrohan I'd like to hear what you think about this proposed scale? If you agree there's a number of ways we can go about rolling this out, and I also sincerely want to avoid extra pressure and work to @gladwearefriends - some options might be delaying v10, doing a minor release shortly after, and @primer/ds-core could take on the work to update too.

Of course another option is that you might feel this updated version doesn't provide you enough benefits and feel the current proposed scale works better - I'm open to that too.

sophshep · 2017-11-06T15:35:13Z

The scale you're proposing looks good to me @broccolini. It will still give us all the values needed for marketing sites. You're right to speak up now vs. after it's shipped and is in use -- not hard to change a few values at this stage in the game!

gladwearefriends · 2017-11-17T20:12:26Z

@broccolini i updated the scale and updated the docs to reflect the new scale as well as remove my poor usage of y-directional everywhere 😬

thank you so much for reading everything carefully and putting thought into it. this was exactly why i wanted to open this PR so we could see the numbers and reflect on the scale more. dont feel bad at all for asking changes later in the game. its better to have a scale that works well for us!

ive learned a lot about how our primer repo is set up and what things to consider when increasing the spacing scale (i.e. emphasis on composability of the values and what primer users would expect (marketing scale should mimic core scale)) so its been a great experience for me :)

broccolini

A couple of bits of feedback, will review more thoroughly when I'm back next week.

broccolini · 2017-11-22T00:43:44Z

modules/primer-marketing-support/README.md

@@ -38,6 +27,31 @@ The source files included are written in [Sass][sass] (`scss`) You can simply po

 You can also import specific portions of the module by importing those partials from the `/lib/` folder. _Make sure you import any requirements along with the modules._

+<!-- %docs
+title:


This needs a title and status:

title: Marketing support
status: new release

And ## Documentation should be before the <!--%docs so that it looks good in the readme when someone views it on dotcom.

broccolini · 2017-11-23T01:18:02Z

modules/primer-marketing-support/README.md

-Documentation & refactor coming very soon
-
-<!-- %enddocs -->
-
 ## Install


Usage instructions should follow ## Install, it probably got missed with other updates 😬. Would you mind adding it? You can copy this section and update to say primer-marketing-support: https://github.com/primer/primer/blob/master/modules/primer-support/README.md#usage

Usage is already included below ## Install here :) https://github.com/primer/primer/pull/376/files/7c8b3be162835cc4d51fedbb9f74d6f9036198ed#diff-e22f2520c00c79678dbb5d56c4e38b6aR20

Oh sorry I missed that!

broccolini · 2017-12-08T18:38:34Z

@gladwearefriends This looks good to go! I'll update your github pr before merging this one in so we can test.

broccolini · 2017-12-11T23:41:24Z

@gladwearefriends confirmed changes look good in github/github so I'm merging this in.

gladwearefriends requested review from broccolini and sophshep October 23, 2017 21:34

gladwearefriends force-pushed the increase-spacing-scale branch from 3b0a701 to 100cc8f Compare October 23, 2017 21:44

sophshep reviewed Oct 25, 2017

View reviewed changes

broccolini mentioned this pull request Oct 25, 2017

Tracking issue for Primer v10 #335

Closed

28 tasks

broccolini added the v10 label Oct 25, 2017

sophshep reviewed Nov 2, 2017

View reviewed changes

gladwearefriends commented Nov 2, 2017

View reviewed changes

gladwearefriends force-pushed the increase-spacing-scale branch from f9dedd2 to 2f38f54 Compare November 2, 2017 22:01

broccolini requested changes Nov 4, 2017

View reviewed changes

jonrohan added the minor release label Nov 6, 2017

broccolini removed the v10 label Nov 6, 2017

broccolini mentioned this pull request Nov 6, 2017

Tracking issue for Primer 10.1.0 #392

Closed

13 tasks

jonrohan mentioned this pull request Nov 16, 2017

Tracking issue for 10.2.0 #406

Closed

12 tasks

gladwearefriends added 12 commits November 17, 2017 12:06

increase spacing scale

ca1c8cd

spacer index off

78c5518

spacer vars wrong number

cb8ef3a

add new variables explicitly, remove x directional spacing from loop

f802e42

-_-

839ce8d

add primer marketing support dependency

3692d95

rm extra line space

d1e3455

forgot to update these scale vars too

e12c3e2

add docs, and mention extended scale in primer-support docs too

728215f

add stories.js for storybook

4b6e983

update docs with new copy and new scale

7ab3261

update scale

2055351

gladwearefriends force-pushed the increase-spacing-scale branch from aff9bf5 to 2055351 Compare November 17, 2017 20:08

use primer-core language and fix links

7c8b3be

broccolini reviewed Nov 27, 2017

View reviewed changes

add title and status to readme

7b55844

broccolini approved these changes Dec 8, 2017

View reviewed changes

jonrohan added the Tag: Enhancement label Dec 11, 2017

jonrohan changed the base branch from dev to release-10.2.0 December 11, 2017 15:44

broccolini merged commit 6ffd8ae into release-10.2.0 Dec 11, 2017

broccolini deleted the increase-spacing-scale branch December 11, 2017 23:42

sophshep mentioned this pull request Jan 18, 2018

Remove primer-page-sections #438

Merged

This was referenced Mar 21, 2018

Bump primer-core from 6.7.0 to 6.8.0 github/opensourcefriday#213

Merged

Bump primer-core from 6.0.0 to 6.8.0 sonde123/opensource.guide#3

Closed

github-actions bot mentioned this pull request Nov 28, 2022

Link Checker Report ronna/MERL-Center-public#20

Open

This was referenced Dec 14, 2022

Link Checker Report ronna/MERL-Center-public#38

Open

Link Checker Report ronna/MERL-Center-public#46

Open

Link Checker Report ronna/MERL-Center-public#48

Open

This was referenced Dec 24, 2022

Link Checker Report ronna/MERL-Center-public#58

Open

Link Checker Report ronna/MERL-Center-public#60

Open

Link Checker Report ronna/MERL-Center-public#66

Open

Link Checker Report ronna/MERL-Center-public#68

Open

This was referenced Jan 7, 2023

Link Checker Report ronna/MERL-Center-public#86

Open

Link Checker Report ronna/MERL-Center-public#92

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Increase spacing scale #376

Increase spacing scale #376

gladwearefriends commented Oct 23, 2017 •

edited

Loading

sophshep left a comment

sophshep Oct 25, 2017

sophshep Oct 25, 2017

gladwearefriends Nov 1, 2017

sophshep Oct 25, 2017

gladwearefriends Nov 1, 2017

sophshep Oct 25, 2017

sophshep Oct 25, 2017

sophshep left a comment

sophshep Nov 2, 2017

gladwearefriends Nov 2, 2017

broccolini Nov 4, 2017

broccolini left a comment

broccolini Nov 4, 2017

broccolini Nov 4, 2017

broccolini Nov 4, 2017

broccolini Nov 4, 2017

broccolini Nov 4, 2017

broccolini Nov 4, 2017

broccolini Nov 4, 2017

broccolini Nov 4, 2017

broccolini Nov 4, 2017

broccolini Nov 4, 2017

gladwearefriends Nov 17, 2017

broccolini commented Nov 4, 2017

sophshep commented Nov 6, 2017

gladwearefriends commented Nov 17, 2017 •

edited

Loading

broccolini left a comment

broccolini Nov 22, 2017

broccolini Nov 23, 2017

gladwearefriends Dec 6, 2017

broccolini Dec 8, 2017

broccolini commented Dec 8, 2017

broccolini commented Dec 11, 2017

		@@ -33,7 +33,7 @@ $ npm run build

		## Documentation


		You can read more about utilities in the [docs folder](./docs/).

		@@ -38,6 +27,24 @@ The source files included are written in [Sass][sass] (`scss`) You can simply po

		You can also import specific portions of the module by importing those partials from the `/lib/` folder. _Make sure you import any requirements along with the modules._

		## Documentation


		Use y-directional utilities to apply margin to the top, bottom or Y axis of an element. Directional utilities can change or override default margins, and can be used with a spacing scale of 7-11.

		Note: The marketing spacing scale extends the core 0-6 scale. See [primer-marketing-support](https://github.com/primer/primer-css/tree/master/modules/primer-marketing-support) for the extended marketing spacing scale.


		## Responsive y-directional margins

		All y-directional margin utilities can be adjusted per [breakpoint](/styleguide/css/modules/grid#breakpoints) using the following formula: `m[y-direction]-[breakpoint]-[spacer]`. Each responsive style is applied to the specified breakpoint and up.


		## Y-Directional margin spacing

		Use y-directional utilities to apply margin to the top, bottom or Y axis of an element. Directional utilities can change or override default margins, and can be used with a spacing scale of 7-11.

Increase spacing scale #376

Increase spacing scale #376

Conversation

gladwearefriends commented Oct 23, 2017 • edited Loading

sophshep left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophshep left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

broccolini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

broccolini commented Nov 4, 2017

sophshep commented Nov 6, 2017

gladwearefriends commented Nov 17, 2017 • edited Loading

broccolini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

broccolini commented Dec 8, 2017

broccolini commented Dec 11, 2017

gladwearefriends commented Oct 23, 2017 •

edited

Loading

gladwearefriends commented Nov 17, 2017 •

edited

Loading