-
Notifications
You must be signed in to change notification settings - Fork 34
docs: adding CSS patterns for typography, grid, etc for developers #2755
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
base: main
Are you sure you want to change the base?
Conversation
|
✅ Deploy Preview for red-hat-design-system ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
…terns' into docs/css-assets-patterns
|
Size Change: 0 B Total Size: 257 kB ℹ️ View Unchanged
|
Co-authored-by: Steven Spriggs <[email protected]>
adamjohnson
left a comment
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.
Is this meant to solve #1432?
| *, | ||
| *:before, | ||
| *:after { | ||
| box-sizing: inherit; /* Make all elements inherit the root's setting */ |
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 see this is the "probably slightly better best practice" you linked to in the PR body; however, when I look at the DP it shows the following:
*,
*:before,
*:after {
box-sizing: border-box;
}Something must not be updating somewhere...?
| } | ||
|
|
||
| /* Remove default margin */ | ||
| :where(body, |
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.
Any reason to do this over:
* {
margin: 0;
}Not saying one way is better than the other. Curious if there's intent here.
| /* Default scroll behavior */ | ||
| html:focus-within { | ||
| scroll-behavior: smooth; | ||
| } |
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.
Instead of doing it this way, you might consider:
@media (prefers-reduced-motion: no-preference) {
html:focus-within {
scroll-behavior: smooth;
}
}Then removing lines 43-57.
This way we explicitly allow it when a user has no-preference and don't need to undo it otherwise. Less bytes down the wire.
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 may consider adding:
- Improving media defaults
- Enable keyword animations (add this to the
no-preferenceblock mentioned below, if applicable)
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.
Pedantic me would love to see these properties alphabetized.
@font-face {
font-family: RedHatDisplay;
font-style: normal;
font-weight: 400;
src: url('path/to/fonts/RedHatDisplay/RedHatDisplay-Regular.woff');
text-rendering: optimizelegibility;
}
/* ...et all... */YMMV.
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.
These tokens should probably have fallbacks baked in, eg:
- line-height: var(--rh-line-height-heading);
+ line-height: var(--rh-line-height-heading, 1.3);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.
Also, consider alphabetizing properties, should it suit you.
|
|
||
| :where(kbd) { | ||
| font-family: var(--rh-font-family-body-text); | ||
| font-size: 1rem; |
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.
Should this be:
| font-size: 1rem; | |
| font-size: var(--rh-font-size-body-text-md, 1rem); |
?
|
|
||
| ### Red Hat CDN | ||
|
|
||
| If you are on a `*.redhat.com` domain, you can use our CDN to access the Red Hat fonts: |
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.
Curiously, you can hit this CSS file in your browser if you go to the URL. That said, the actual .woff2 files are blocked by a CORS error for non-Red Hat domains.
| If you are on a `*.redhat.com` domain, you can use our CDN to access the Red Hat fonts: | ||
|
|
||
| ```html code-block {dedent: true, language: "css", highlighting: "prerendered"} | ||
| https://www.redhatstatic.com/dssf-001/v2/@redhat/[email protected]/font.min.css |
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.
Should this be wrapped in a <link> tag? Or are people meant to copy-paste this URL and see its contents?
<link rel="stylesheet" href="https://www.redhatstatic.com/dssf-001/v2/@redhat/[email protected]/font.min.css">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.
This is over 2,000 lines and will get out of date if we ever change rhx-grid.
I would recommend removing this source file in favor of referencing the file in the repo.
Then, add example HTML to show what would be required to use this grid. Something like:
<link rel="stylesheet" href="../path/to/rhx-grid.min.css">
...
<div class="rhx-grid" data-rhx-cols="2xs:2 xs:3 md:4">
<div>Grid Item 1</div>
<div>Grid Item 2</div>
<div>Grid Item 3</div>
<div>Grid Item 4</div>
</div>
What I did
CSS foundationssection to the Get Started: Developers guide with foundational CSS patterns for typography, grid/layout, etc.typography.cssto itsflowlayer instyles.css, since it seems more appropriate.reset.cssto follow a "probably slightly better best practice"Testing Instructions
Notes to Reviewers