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

[Primitives v8] UI color docs #720

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

[Primitives v8] UI color docs #720

wants to merge 17 commits into from

Conversation

langermank
Copy link
Contributor

@langermank langermank commented Feb 6, 2024

Primer color modes

✨ New color tokens, new docs! ✨

This PR adds a new folder for color docs, including the following pages:

  • Overview
  • Base scales
  • Accessibility

The main goal for this PR is to ensure the Overview page is sufficient for staff shipping Primitives v8. I want to make sure that Hubbers have access to docs as we introduce them to the new system. Please note the Accessibility page is a copy/paste from the old colors docs, and I plan to update it with more content and images later.

I also plan to add a page dedicated to explaining Primer themes. Just need more time to work on that and we are staff shipping soon 😄

ALSO note I will add alt text to images once they are finalized and approved by you all!

Feedback

Looking for the following..

  • Grammar/language
  • Graphics
  • Any missing content? More you want to see?

@langermank langermank marked this pull request as ready for review February 15, 2024 01:34
@langermank langermank requested a review from a team as a code owner February 15, 2024 01:34
Copy link
Contributor

@tbenning tbenning left a comment

Choose a reason for hiding this comment

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

This is looking great so far, I can tell you've put a lot of work into it. I left some ideas / suggestions 🚀

One more thing:
Could we make the iframes for the storybooks at foundations/primitives/color the same height as the tables inside of them? The double scroll feels like a pain IMO. Maybe we could do this for foreground background and border since those are probably most frequently referenced.

image

content/foundations/color/overview.mdx Outdated Show resolved Hide resolved
content/foundations/color/overview.mdx Outdated Show resolved Hide resolved
Comment on lines 48 to 53
<img
width="960"
alt="wip"
src="https://github.com/primer/react/assets/18661030/200cd7f0-91dd-4e4e-b574-26612bc191b8"
style="margin-bottom: 1rem; margin-top: 1rem;"
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: I wonder if in this image, we could add the same purple text label that was used in the previous example and show Component/pattern

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both examples are the same type, does this image still read that way? Great suggestion.

pattern color example

content/foundations/color/base-scales.mdx Show resolved Hide resolved

import {Box} from '@primer/react'

## Overview
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion V2:
I wonder if we can have some sort of quick links to help users navigate and find helpful pages regarding color. If I instinctively click color and am looking for the reference, I might not think to look under primitives. Probably a V2, I just wonder if we've considered a pattern like this in our docs before to help with navigation / IA.

Maybe something like:
3 cards UI


<img
width="960"
alt="wip"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is now the right time to start to update these wip alt texts?

content/foundations/color/overview.mdx Show resolved Hide resolved
Copy link

@dipree dipree left a comment

Choose a reason for hiding this comment

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

Left a few comments, almost there!

One thing I was wondering about is: Do we have a good way to keep images up to date when they are using specific parts of the GitHub UI as examples?

content/foundations/color/overview.mdx Outdated Show resolved Hide resolved
content/foundations/color/overview.mdx Outdated Show resolved Hide resolved
content/foundations/color/accessibility.mdx Show resolved Hide resolved
content/foundations/color/accessibility.mdx Show resolved Hide resolved

### Assure adequate color contrast

Color contrast between text and its background must meet [required WCAG standards](https://www.w3.org/WAI/WCAG21/Understanding/contrast-minimum.html).
Copy link

Choose a reason for hiding this comment

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

WCAG recommends to use WCAG22 standard. It's the same link but with 22 -> https://www.w3.org/WAI/WCAG22/Understanding/contrast-minimum.html.

Might be worth to ask Tetralogical for review as they are working on a11y documentation at the moment (cc @alliethu)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! This is all copy/paste from the original color docs. Let's take the time to get it right even if we're just moving it to a new page 👍

Copy link
Contributor

@ichelsea ichelsea left a comment

Choose a reason for hiding this comment

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

All set for a copy/paste in this movement on the color accessibility docs! (Once the link is updated to WCAG 2.2)

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.

None yet

4 participants