Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix marketing utilities #1002

Merged
merged 1 commit into from
Jan 23, 2020
Merged

Fix marketing utilities #1002

merged 1 commit into from
Jan 23, 2020

Conversation

simurai
Copy link
Contributor

@simurai simurai commented Dec 20, 2019

This uses all spacers (0-12) to generate the marketing margin/padding utilities.

It should fix an issue where you couldn't use mb-9 mb-md-4, see https://github.com/github/site-design/issues/809.

Concerns

The marketing bundle would increase by 13.05 K (1.4 K gzipped).

┌─────────────────────┬───────────┬───────┬───────────┬──────────┬──────────┬───────────┬──────────────────────────────┐
│ name                │ selectors │     ± │ gzip size │        ± │ raw size │         ± │ path                         │
├─────────────────────┼───────────┼───────┼───────────┼──────────┼──────────┼───────────┼──────────────────────────────┤
│ primer              │      3703 │ + 315 │   28.74 K │ + 1.23 K │ 186.49 K │ + 13.05 K │ dist/primer.css              │
│ core                │      2254 │     0 │   17.95 K │        0 │ 114.15 K │         0 │ dist/core.css                │
│ utilities           │      1381 │     0 │    8.75 K │        0 │  66.76 K │         0 │ dist/utilities.css           │
│ product             │       474 │     0 │    6.36 K │        0 │  31.15 K │         0 │ dist/product.css             │
│ marketing           │       975 │ + 315 │    5.41 K │  + 1.4 K │  41.08 K │ + 13.05 K │ dist/marketing.css           │
│ marketing-utilities │       933 │ + 315 │     4.4 K │ + 1.38 K │  36.84 K │ + 13.05 K │ dist/marketing-utilities.css │

That's an increase of about 35%? 🤔 Let's see how it compares when moving the marketing spacers to core: #1003

/cc @tobiasahlin


Fixes https://github.com/github/site-design/issues/809

@vercel
Copy link

vercel bot commented Dec 20, 2019

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/primer/primer-css/dzfqmzu76
🌍 Preview: https://primer-css-git-marketing-utilities.primer.now.sh

@simurai simurai marked this pull request as ready for review December 24, 2019 01:08
@tobiasahlin
Copy link
Contributor

Any ETA on this PR? 😊 (just got bitten by this again today 😅 )

@simurai simurai requested a review from jonrohan January 23, 2020 02:24
@simurai
Copy link
Contributor Author

simurai commented Jan 23, 2020

Any ETA on this PR? 😊 (just got bitten by this again today 😅 )

This PR is currently waiting for an approval.

/cc @jonrohan or if anyone in @primer/ds-core wants to take a look 🙇

@jonrohan
Copy link
Member

I’ll take a closer look in an hour. Want to make sure this is the right way to go.

@jonrohan
Copy link
Member

Is the marketing spacer m-2 the same size as the core spacer m-2 ?

The other thought I had when we hit this bug that we could spilt apart the imports for the normal and responsive utilities. Then in .com include them like this:

@import "@primer/css/core/utilities/margin.scss";
@import "@primer/css/marketing/utilities/margin.scss";
@import "@primer/css/core/utilities/margin-responsive.scss";
@import "@primer/css/marketing/utilities/margin-responsive.scss";

Not sure if that's a better idea or not. I suppose the increase isn't really big. 🤔

@simurai
Copy link
Contributor Author

simurai commented Jan 23, 2020

Is the marketing spacer m-2 the same size as the core spacer m-2 ?

Right. On marketing pages there will be two m-2 with the same value.

  • m-2 from frameworks.css
  • m-2 from site.css <- this is needed so responsive variants can override the 7-12 sizes. e.g. m-9 m-sm-2

The other thought I had when we hit this bug that we could spilt apart the imports for the normal and responsive utilities.

Yeah, might be an option. Although then, would we also need to remove these imports?

// in src/utilities/index.scss
@import "./margin.scss";
@import "./padding.scss";

Otherwise the margin + padding utilities would still be included when importing the core bundle on dotcom.

🤔 Hmmm... Next to core, product and marketing bundles, we would kinda create separate and new bundles only for margin/padding which might be a bit overkill?

Copy link
Member

@jonrohan jonrohan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the best way to go 👍🏻 At least I’m pretty sure I don’t have a better idea.

@simurai
Copy link
Contributor Author

simurai commented Jan 23, 2020

Yeah, it's maybe the best of all the bad options. 🙈 😬 Or at least it avoids having to rethink/refactor how we currently bundle and serve CSS to each page.

@simurai simurai changed the base branch from master to release-14.2.0 January 23, 2020 04:57
@simurai simurai merged commit f914053 into release-14.2.0 Jan 23, 2020
@simurai simurai deleted the marketing-utilities branch January 23, 2020 04:57
@simurai simurai mentioned this pull request Jan 23, 2020
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants