-
Notifications
You must be signed in to change notification settings - Fork 535
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
Update docs to use functional color variables #1111
Conversation
Tweak color in sidenav
🦋 Changeset detectedLatest commit: 360e359 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/primer/primer-components/JB1zrtNhGP95HmLpdhWdsHBjUxHY |
@@ -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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 👍
| 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 | |
There was a problem hiding this comment.
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.
docs/content/Box.md
Outdated
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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
Tweak color in timeline timeline tweaks
72508cc
to
d204e73
Compare
docs/content/Box.md
Outdated
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> |
There was a problem hiding this comment.
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.
Co-authored-by: Cole Bemis <colebemis@github.com>
docs/content/ProgressBar.mdx
Outdated
| 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 | |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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
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