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

Color system docs updates #811

Merged
merged 48 commits into from
Jul 17, 2019
Merged

Color system docs updates #811

merged 48 commits into from
Jul 17, 2019

Conversation

shawnbot
Copy link
Contributor

@shawnbot shawnbot commented May 22, 2019

@vercel
Copy link

vercel bot commented May 22, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

@shawnbot
Copy link
Contributor Author

@broccolini @emilybrick Here's where I got with the color utility tables today:

image

Unfortunately, they're not very responsive on more narrow viewports:

image

I'm using fixed-layout HTML tables right now, but we could switch to wrapped flexbox and vary widths by screen size for nicer layouts at smaller widths. Aside from the layout, I could really use some guidance on how to name the columns. 😬

@simurai
Copy link
Contributor

simurai commented May 31, 2019

layouts at smaller widths

Hmm.. 🤔 Maybe still combine the default and index classes into a single column? I think that's also more to @emplums's liking? Something like this:

📱 🖥
Screen Shot 2019-05-31 at 4 17 24 PM Screen Shot 2019-05-31 at 4 17 47 PM

It makes it look like the default classes are more like "alternatives" and harder to see. Maybe fine if we add a note about it at the beginning.

@emilybrick
Copy link
Contributor

@shawnbot I actually think your latest iteration makes it quite clear, even if a little unbalanced looking to only have a few items in the first column.

I agree why you'd want to combine @simurai, but imo it dilutes the value of the default classes, and makes it less clear that we'd like people to use them before resorting to the extended (numbered/indexed) classes.

In terms of naming, my vote would be for
Default Classes | Extended Classes | Variable | CSS Value

maybe we have some small explanatory text (in lieu of a tooltip) underneath the headings.

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.

Wrapping seems improved a lot. 👍

The hex values get hidden at some point on narrow widths, but maybe ok, considering the audience. 🤷‍♂ One thing we could consider is removing the 3rd column with the variables of the background colors. E.g. $gray-000. Might not really be needed once you look up the patterned under "Color System"?

@skullface
Copy link
Contributor

I love this new styling 😍

@shawnbot
Copy link
Contributor Author

Okay, so I have waffled a bit on what to put where in this PR, and I think I've settled on at least the right data to show on each page (as of a24d1b6):

Color system

@broccolini I tried bringing the more verbose tables over into this page per our chat, and it just didn't work. I think it makes more sense to keep the existing layout of this page, but bring the color tables closer in appearance to the ones on other pages (see below):

image

Color utilities / Backgrounds

image

Color utilities / Text colors

The problem I was trying to solve with the underline was that without a background color it's hard to visually connect the "default" or "alias" class (.text-blue) with its numbered equivalent (.color-blue-5):

image

Border colors

I had to separate the table cells so that adjacent borders didn't touch on this one:

image

What's up with the zaps?

The icons are very much FPO, and are meant to indicate "preferred" or "default" class aliases that people should use rather than the numbered ones. Maybe these could be tooltipped or link to a note at the bottom of the page?

@shawnbot
Copy link
Contributor Author

The hex values get hidden at some point on narrow widths, but maybe ok, considering the audience. 🤷‍♂ One thing we could consider is removing the 3rd column with the variables of the background colors. E.g. $gray-000. Might not really be needed once you look up the patterned under "Color System"?

Yeah, I was thinking we could selectively hide a column or two at smaller widths.

@shawnbot
Copy link
Contributor Author

Another thought is that we could unify the color group heading styles so that e.g. “Gray” looks the same everywhere and has an obvious permalink icon.

@sophshep
Copy link

🚗 drive-by comment just to say I love how this sets up the relationship between the preferred class / alias class / variable / hex!

@shawnbot shawnbot changed the base branch from release-12.4.1 to master June 21, 2019 17:57
@mxstbr
Copy link
Contributor

mxstbr commented Jun 27, 2019

Love this so much, it's going to make my life so much easier!

While implementing designs with @primer/components, I end up looking at the theme doc and CMD+F searching for the HEX values from Figma to find the right names in the theme object. That forces me to count how many colors there are in that array to figure out whether to put grey.5 or grey.6 in my code.

Maybe out of scope, but it would be awesome if this page had a client-side search input that I could paste a HEX value in and it would spit out the color name for me to use in the code!

@simurai simurai mentioned this pull request Jul 2, 2019
16 tasks
@simurai
Copy link
Contributor

simurai commented Jul 17, 2019

I think there were some discussions about eventually moving this overview somewhere else? But let's merge this for now since it seems very useful to have. 🚢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants