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

Update docs to use functional color variables #1111

Merged
merged 25 commits into from
Mar 9, 2021

Conversation

VanAnderson
Copy link
Contributor

@VanAnderson VanAnderson commented Mar 5, 2021

This PR updates the documentation to remove references to old color values.

It also updates some language to make the language around colors more functional and less specific to a particular theme. This work is concentrated in the commit d204e73.

There are a few area's that still need attention, putting some notes on this one.

Part of #1108

@changeset-bot
Copy link

changeset-bot bot commented Mar 5, 2021

🦋 Changeset detected

Latest commit: 360e359

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

This PR includes changesets to release 1 package
Name Type
@primer/components Patch

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

@vercel
Copy link

vercel bot commented Mar 5, 2021

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

🔍 Inspect: https://vercel.com/primer/primer-components/JB1zrtNhGP95HmLpdhWdsHBjUxHY
✅ Preview: https://primer-compone-git-vananderson-update-docs-for-functiona-0474b6.vercel.app

docs/content/Position.md Outdated Show resolved Hide resolved
docs/content/Timeline.md Outdated Show resolved Hide resolved
@@ -2,17 +2,17 @@
title: Overriding styles with the sx prop
---

Our goal with Primer React is to hit the sweet spot between providing too little and too much styling flexibility; too little and the design system is too rigid, and too much and it becomes too difficult to maintain a consistent style. Our components already support a standard set of [system props](/system-props), but sometimes a component just isn't *quite* flexible enough to look the way you need it to look. For those cases, we provide the `sx` prop.
Our goal with Primer React is to hit the sweet spot between providing too little and too much styling flexibility; too little and the design system is too rigid, and too much and it becomes too difficult to maintain a consistent style. Our components already support a standard set of [system props](/system-props), but sometimes a component just isn't _quite_ flexible enough to look the way you need it to look. For those cases, we provide the `sx` prop.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

My default linter is really going to town on some of these files... not sure if I should try to tone it down?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine 👍

Comment on lines +23 to +28
| Name | Type | Default | Description |
| :------------ | :-------- | :-----------: | :----------------------------------------------------------------------------------------------------------- |
| ariaLabel | String | | Specifies the `aria-label` attribute, which is read verbatim by screen readers |
| icon | Component | | [Octicon component](https://github.com/primer/octicons/tree/master/lib/octicons_react) used in the component |
| size | Number | 16 | Sets the uniform `width` and `height` of the SVG element |
| verticalAlign | String | `text-bottom` | Sets the `vertical-align` CSS property |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

linter is really working overtime, but I think the extra spacing here makes it much easier to read in an IDE.

Comment on lines 11 to 22
Box can be used to create both{' '}
<Box as="span" bg="bg.success">
inline
</Box>{' '}
and
<Box bg="bg.danger">block-level elements,</Box>
<Box bg="bg.warning" width={[1, 1, 1 / 2]}>
elements with fixed or responsive width and height,
</Box>
<Box bg="bg.info" p={4} mt={2}>
and more!
</Box>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some of these functional color choices I was trying out end up being very light shades - we could maybe consider switching to [success/danger/info]Inverse?

Screen Shot 2021-03-05 at 11 08 32 AM

Screen Shot 2021-03-05 at 11 08 43 AM

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. I think using the *Inverse colors for Box, Grid, and Flex examples is fine. We'd probably need to pair that with color="text.inverse" as well.

@VanAnderson VanAnderson mentioned this pull request Mar 5, 2021
8 tasks
@VanAnderson VanAnderson force-pushed the VanAnderson/update-docs-for-functional-vars branch from 72508cc to d204e73 Compare March 5, 2021 17:33
@vercel vercel bot temporarily deployed to Preview March 5, 2021 17:36 Inactive
Comment on lines 11 to 22
Box can be used to create both{' '}
<Box as="span" bg="bg.success">
inline
</Box>{' '}
and
<Box bg="bg.danger">block-level elements,</Box>
<Box bg="bg.warning" width={[1, 1, 1 / 2]}>
elements with fixed or responsive width and height,
</Box>
<Box bg="bg.info" p={4} mt={2}>
and more!
</Box>
Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. I think using the *Inverse colors for Box, Grid, and Flex examples is fine. We'd probably need to pair that with color="text.inverse" as well.

docs/content/LabelGroup.md Outdated Show resolved Hide resolved
docs/content/Position.md Outdated Show resolved Hide resolved
docs/content/SideNav.md Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview March 8, 2021 19:57 Inactive
Co-authored-by: Cole Bemis <colebemis@github.com>
@vercel vercel bot temporarily deployed to Preview March 8, 2021 20:02 Inactive
@VanAnderson
Copy link
Contributor Author

image

yeah, much nicer!

@VanAnderson VanAnderson marked this pull request as ready for review March 8, 2021 20:03
@vercel vercel bot temporarily deployed to Preview March 8, 2021 20:06 Inactive
| progress | Number | | Used to set the size of the green bar |
| barSize | String | 'default' | Controls the height of the progress bar. Can be 'small', 'large', or 'default'. If omitted, height is set to the default height. |
| inline | Boolean | false | Styles the progress bar inline |
| bg | String | 'state.success' | Set the progress bar color, defaults to bg-green |
Copy link
Contributor

Choose a reason for hiding this comment

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

I just noticed that state.success is not a color that's defined in Primer Primitives. Let's see if we can find an alternative.

Copy link
Contributor

Choose a reason for hiding this comment

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

bg.successInverse might be the closest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

yep, looks good to me!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated both docs and component in cbe8ca4

@vercel vercel bot temporarily deployed to Preview March 8, 2021 20:58 Inactive
@VanAnderson VanAnderson merged commit 057a489 into main Mar 9, 2021
@VanAnderson VanAnderson deleted the VanAnderson/update-docs-for-functional-vars branch March 9, 2021 20:02
@github-actions github-actions bot mentioned this pull request Mar 9, 2021
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.

2 participants