Merge overlapping marketing spacing utilities into primer core

[tobiasahlin]

Per discussion in #1242, this PR outlines a merge of the overlapping marketing spacing utilities into primer core. Method in brief:

• I'm renaming the $marketing-all-spacers map to $spacer-map-extended, and moving it to variables/layout.scss for use across all of primer, not only marketing
• I'm extending margins only along the y axis (y, t, b) with this map, just like the marketing utilities currently does
• I'm looping through the $spacer-map-extended map for all margin and padding core utilities, but limiting the extended scale for X-axis margin utilities by checking for the length of the $spacer-map (@if ($scale < length($spacer-map)) { … })

Todo

• Agree on suitable implementation
• Update documentation to reflect the changes

[changeset-bot]

🦋 Changeset detected

Latest commit: 8eafb83

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/css	Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[simurai]

Did we come to a conclusion in #1242? As far as I understood it:

• Pro for marketing: Fixes the "overriding" problem + reduces file size.
• Pro for product: Extended spacing scale with additional 48px - 128px spacing.
• Con for product: File size increase.

Re file size increase... Did a quick local test of dotcom:

Before	After

• Product pages (frameworks.css) increase by +1.7kB.
• Marketing pages (frameworks.css + site.css) decrease by -1.9kb. Since it's not duplicated anymore.

So I would say maybe fine to move forward. 👍 Especially as a stop gap until we rethink all the spacing with https://github.com/github/primer/issues/41.

/cc @jonrohan @vdepizzol Any objections or concerns?

[tobiasahlin]

I realized that I had missed limiting the m- utilities (margin on all sides) to the default scale (fixed above), so this should reduce the added size to frameworks.css a bit 🙏

Edit: I made the same change in padding.scss, and limited the utility classes that adds paddings on all sides to the default scale. This is in fact a deprecation, so I added these to deprecations.js, but there are 0 instances of these on .com, which makes me question their usefulness. Given that, and given that we can choose to be consistent with the margin setup, I vote for deprecating them as outlined in the commit below. This should further decrease the extra load on frameworks.scss.

I've also drafted how these changes can be reflected in the documentation 🙏 it's worth noting that uniform padding utilities (p-7 and upwards) were never documented, so this documentation didn't need to be updated with the deprecation.

[tobiasahlin]

Here's an updated comparison with the latest changes (these numbers are not gzipped and in production, but rather the file sizes of the bundles on disk):

Bundle	main	This branch	Diff
utilities.css	68 kb	82 kb	+14 kb
marketing-utilities.css	43 kb	12 kb	-29 kb

I.e. we're removing about 15kb of CSS in total.

[simurai]

Should we move all of marketing/utilities/margin.scss? It's almost not worth to keep the marketing margin utilities for only these two classes.

And I think the auto margin are handy even for product, we briefly considered that in https://github.com/github/design-systems/issues/550#issuecomment-460691862. Also lots of custom styled margin: 0 auto to center layouts.

Could also be a separate PR to keep this one focused.

[simurai]

but there are 0 instances of these on .com, which makes me question their usefulness. Given that, and given that we can choose to be consistent with the margin setup, I vote for deprecating them as outlined in the commit below.

That sounds reasonable to me. 👍 And we could add them back in case it gets requested a lot.

[tobiasahlin]

Should we move all of marketing/utilities/margin.scss? It's almost not worth to keep the marketing margin utilities for only these two classes.

@simurai I did this now in a commit above to clean up this file completely ✨

Could also be a separate PR to keep this one focused.

This is also a fair point, I'd be happy to revert this last change, too, if we feel that we're including too much in this PR now 🙏

[simurai]

I'd be happy to revert this last change, too, if we feel that we're including too much in this PR now

Move sounds good for now. 👍 We can consider follow-up PRs to expand the auto margin a bit more. Like having a mx-auto or so.