-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
New utilities & docs - fade out, hover grow, border white fade, responsive positioning, and circle #375
Conversation
|
||
```html | ||
<div class="box hover-grow p-4"> | ||
<%= octicon("mark-github"), :height => 32 %> |
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 the )
is misplaced here; it should go after 32
.
FYI @broccolini, @sophshep and I paired on getting these additions into Storybook, and ended up adding all of the utilities in the process. There are a lot of code blocks in the |
@sophshep I pulled this down and I don't want to publish storybook like this with all the garbled stories titles in the nav, and having everything under utilities makes it hard to navigate as well as super long. Did you and @shawnbot discuss any options for fixing this up? I can help with this next week if needed but wanted to check what you'd discussed first. |
No, we didn't. I used the story mainly to test the components I wrote for the docs. Could we remove it from this PR and then fix separately when adding stories to all the other styles? |
@broccolini I've updated storybook to manually add the new styles that are in this PR. |
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.
Totes on board with these additions and happy to see custom Sass being remove from GitHub. Requested changes mostly re documentation feedback.
@@ -15,3 +15,8 @@ | |||
.border-#{$breakpoint}-left-0 { border-left: 0 !important; } | |||
} | |||
} | |||
|
|||
/* Use with .border to turn the border rgba white 0.15 */ | |||
.border-white-fade { |
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.
Small code-style comment, we usually put single line properties on one line 😬
|
||
### White border with alpha transparency | ||
|
||
In addition to `.border-black-fade` which is in primer-core, primer-marketing includes `.border-white-fade`. This adds a rgba white border with an alpha transparency of `0.15`. This is useful when you want a border that is a lighter shade of the background color. |
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 starting off with what this style is and how it can be used is more helpful, and I don't think you need to specifically reference the border-black-fade
specifically but would be nice to link to the border color utilities. If you do think it's important to mention the black version, perhaps say "such as border-black-fade
in the last sentence? Feel free to edit:
Use
border-white-fade
to add a white border with an alpha transparency of0.15
. This is useful when you want a border that is a lighter shade of the background color. Additional border colors are available in primer-core border utilities.
(I think this is the right url but worth double checking in the style guide.)
@@ -17,3 +17,22 @@ Top, right, bottom, and left border utilities are can be used responsively to ad | |||
.border-top-0 | |||
</div> | |||
``` | |||
|
|||
## Border Colors |
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.
Does this need to be changed to Marketing border colors
to avoid dupes in stories?
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 have these added manually to the story for now, using the class names. But they're under "marketing utilities" so I think it's pretty clear.
--- | ||
title: Layout | ||
status: New release | ||
status_issue: https://github.com/github/design-systems/issues/325 |
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.
We're moving away from status issues and plan to just let people open a new issue in the Primer repo so that it will work when this goes public.
status_issue: https://github.com/github/design-systems/issues/325 | ||
--- | ||
|
||
The following layout utilities are meant to used in addition to those within primer-core. |
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.
The first paragraph should describe what these styles are, this para will get used in other contexts such as og metadata in future. Also, it's nice to hyperlink things that are documented in the style. Suggest something like this:
Marketing layout utilities build on top of primer-core utilities, adding the option of responsive positioning.
|
||
## Responsive position | ||
|
||
These position utilities behave the same way as the positioning utilities in primer-core, however they include responsive states so that they can be used at specific viewport widths. |
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.
It's good to start with how to use the style before getting into details such as referencing how it works with other styles. I basically try and write docs with the assumption that people will put up with reading information for about 5 seconds 🙃. Suggest something like:
Use responsive position utilities to adjust the position of an element at different breakpoints. These can be used in addition to the position utilities available in primer-core.
@@ -109,7 +109,7 @@ Use `border-dashed` to give an element a dashed border. | |||
|
|||
## Rounded corners | |||
|
|||
Add or remove rounded corners: `rounded-0` removes rounded corners, `rounded-1` applies a border radius of 3px, `rounded-2` applies a border radius of 6px. | |||
Add or remove rounded corners: `rounded-0` removes rounded corners, `rounded-1` applies a border radius of 3px, `rounded-2` applies a border radius of 6px. `.circle` applies a border radius of 50%, which turns square elements into perfect circles. |
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 realize this wasn't your edit, but could we start with a slightly nicer intro, such as:
Use the following utilities to add or remove rounded corners:
@@ -151,3 +172,13 @@ | |||
transform: scale3d(1, 1, 1); | |||
} | |||
} | |||
|
|||
/* Increase scale of an element on hover */ | |||
|
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.
Accidental extra white space?
This PR adds the following styles and corresponding docs to Primer:
Fade out animation
This is in github/github but not in Primer nor was it documented. (this will close https://github.com/github/design-systems/issues/424)
Hover-grow animation
This is a pattern used in many places, for example on Explore spotlights, our customers page, and even on our style guide homepage. Since it's used on so many different types of projects, I decided to put it in core utilties vs. marketing.
border-white-fade
Similar to
border-black-fade
, but to be used on dark backgrounds. I needed this on these video pages and it could also be used on marketing sites with dark backgrounds or components, such as the partner site.Responsive Positioning
This is something I have needed on a few projects and added as custom CSS in github/github. For now I am leaving this in marketing only, but as we move forward with more responsive stuff in product we may want to move it over.
(note: I have never needed
position-static
, let alone a responsive version of it, so I left it out to prevent unnecessary CSS. Any objections to this or should responsive styles always have full parity with their non-responsive counterparts?).circle
Which adds a border-radius of 50%. This was brought up in this issue, and is needed in both marketing and product.
/cc @primer/ds-core
To-do for v10: