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

More color utilities! #760

Merged
merged 4 commits into from Apr 30, 2019
Merged

More color utilities! #760

merged 4 commits into from Apr 30, 2019

Conversation

shawnbot
Copy link
Contributor

@shawnbot shawnbot commented Apr 18, 2019

This PR adds 140 new utility classes — a foreground and background utility for each shade (10) of every hue (of which there are 7).

There is some redundancy with existing utilities, and GitHub's postcss config combines selectors with the same style (for instance, .text-blue and .fg-blue-5 are combined), but the size difference is not actually pretty insignificant: less than 1K gzipped!

┌─────────────────────┬───────────┬───────┬───────────┬─────────┬──────────┬───────┬──────────────────────────────┐
│ name                │ selectors │     ± │ gzip size │       ± │ raw size │     ± │ path                         │
├─────────────────────┼───────────┼───────┼───────────┼─────────┼──────────┼───────┼──────────────────────────────┤
│ primer              │      3096 │ + 140 │   23.88 K │ + 991 B │ 151.95 K │ + 6 K │ dist/primer.css              │
│ core                │      2139 │ + 140 │   17.12 K │ + 998 B │ 108.09 K │ + 6 K │ dist/core.css                │
│ utilities           │      1292 │ + 140 │    8.23 K │ + 998 B │  62.73 K │ + 6 K │ dist/utilities.css           │

I am swapping out the text- prefix used in our "old" color utilities for fg- color- because we often use utilities to give color to octicons rather than text specifically.

@shawnbot shawnbot requested a review from a team April 18, 2019 23:27
@shawnbot shawnbot marked this pull request as ready for review April 18, 2019 23:34
@simurai
Copy link
Contributor

simurai commented Apr 24, 2019

so a 6k increase before gzip

Since these styles are very repetitive, it might not be that big of a difference after gzipping? 🤔

Copy link
Contributor

@simurai simurai left a comment

Choose a reason for hiding this comment

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

I'm on the fence with the fg- vs color- prefix.

  • fg- is short and would make it similar to bg-.
  • color- describes better what it does -> changing the color property.

Hmm.. I'm slightly leaning towards color-.

@shawnbot
Copy link
Contributor Author

Since these styles are very repetitive, it might not be that big of a difference after gzipping? 🤔

You're right! Before: 7.48 kB vs. after: 8.47 kB for dist/utilities.css. I got this with:

git co master
npm run dist
npx gzip-size-cli dist/utilities.css
# 7.48 kB
git co more-color-utilities
npm run dist
npx gzip-size-cli dist/utilities.css
# 8.47 kB

So yeah, maybe not as big of an issue as I thought! 🤗

@shawnbot
Copy link
Contributor Author

shawnbot commented Apr 24, 2019

I'm on the fence with the fg- vs color- prefix.

Yeah, I went back and forth on this. The use case that has me leaning toward fg- is that people do and will use these classes to color octicons (and other SVG elements), which has always felt a little awkward/unintuitive to me:

<%= octicon("heart", class: "text-red") %>

...thanks to .octicon { fill: currentColor; }.

Of course, on the other hand, color- matches up with the styled-system props in Primer Components, so that alone might make it preferable. @jonrohan @broccolini @emplums, any thoughts?

@simurai
Copy link
Contributor

simurai commented Apr 24, 2019

On a second thought, because color is something used quite often, how about just a c-. Similar to the m- or p- utilities.

<%= octicon("heart", class: "color-red-8") %>
<%= octicon("heart", class: "fg-red-8") %>
<%= octicon("heart", class: "c-red-8") %>

Not sure how much trouble it would be to also change it for Components?

<StyledOcticon icon={Heart} color="red.8" />
<StyledOcticon icon={Heart} fg="red.8" />
<StyledOcticon icon={Heart} c="red.8" />

@shawnbot shawnbot force-pushed the more-color-utilities branch 3 times, most recently from ea6e0c2 to 2db3b0b Compare April 26, 2019 23:20
@shawnbot shawnbot mentioned this pull request Apr 26, 2019
@emplums
Copy link
Contributor

emplums commented Apr 29, 2019

Just copying my Slack comments over here!

I could see the benefit of either fg- or color-. I feel like fg is more descriptively correct, but color lines up with our React component API better and I think is more intuitive - not everyone would recognize that fg means foreground or know what that means in the context of CSS. And it's literally changing just the color CSS rule so color does feel better in that sense!

@shawnbot shawnbot mentioned this pull request Apr 29, 2019
13 tasks
@shawnbot shawnbot changed the base branch from master to release-12.3.0 April 29, 2019 18:40
7: $purple-700,
8: $purple-800,
9: $purple-900,
) !default;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: I did this by hand, but we're going to eventually automate the generation of this file from primer-colors, as in #761.

Copy link
Contributor

@simurai simurai left a comment

Choose a reason for hiding this comment

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

On Slack the consensus was to go with color- prefix. Therefore ready to 🚢 👍

@simurai simurai moved this from Needs review to Ready to release in 📦 Primer CSS release tracking Apr 30, 2019
@shawnbot shawnbot merged commit e550970 into release-12.3.0 Apr 30, 2019
📦 Primer CSS release tracking automation moved this from Ready to release to 💜 Done Apr 30, 2019
@shawnbot shawnbot deleted the more-color-utilities branch April 30, 2019 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants