-
-
Notifications
You must be signed in to change notification settings - Fork 10
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
refactor: consolidate CSS stylesheets #114
refactor: consolidate CSS stylesheets #114
Conversation
*/ | ||
border-width: 0; | ||
html { | ||
@apply text-md; |
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 are two font sizes specified in global-element-styles
(the latter takes precedence):
ui/src/global-element-styles.css
Lines 66 to 67 in 7f9fc26
html { font-size: 10px; ui/src/global-element-styles.css
Lines 377 to 379 in 7f9fc26
html { height: 100%; font-size: 18px;
And since we want to use the standard styles for html
text:
ui/src/global-element-styles.css
Lines 375 to 376 in 7f9fc26
/* Element styles rewrites from global.css that could be moved to presets*/ | |
/* typography should be handled in tailwind config */ |
I applied the text-md
style to the element.
The text-md
style is defined as follows:
Line 98 in 7f9fc26
md: ["18px", "1.42857143"], |
Also, we don't need to specify font family here.
In the Tailwind preflight stylesheet:
html
gets thesans
font: https://github.com/tailwindlabs/tailwindcss/blob/f1f419a9ecfcd00a2001ee96ab252739fca47564/src/css/preflight.css#L36code
,kbd
,samp
, andpre
get themono
font: https://github.com/tailwindlabs/tailwindcss/blob/f1f419a9ecfcd00a2001ee96ab252739fca47564/src/css/preflight.css#L115
sans
and mono
fonts are specified here:
Lines 90 to 93 in 7f9fc26
fontFamily: { | |
sans: ["Lato", "sans-serif"], | |
mono: ["Hack-ZeroSlash", "monospace"], | |
}, |
/* Override the browser default border width in order to style individual border sides | ||
* Ref: https://stackoverflow.com/a/76961084 | ||
*/ | ||
border-width: 0; |
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 needed to add this rule because we didn't use Tailwind preflight. It can be removed now that we enable the preflight.
Ref: https://github.com/tailwindlabs/tailwindcss/blob/3.4/src/css/preflight.css#L10
:focus-visible { | ||
@apply outline outline-3 outline-focus-outline-color outline-offset-0; | ||
} |
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 a custom rule.
I make it global so that we don't have to manually add it into each component. This also ensures that the focus outline display is consistent across components.
code { | ||
@apply bg-background-tertiary text-foreground-tertiary; | ||
} | ||
:not(pre) > code { | ||
@apply border-1 border-gray-450; | ||
} |
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.
When I was checking the Tabs component on Storybook (/?path=/story/components-tabs--default), I noticed that the text color of the code
element is red:
This comes from:
ui/src/global-element-styles.css
Lines 267 to 273 in 7f9fc26
code { | |
padding: 2px 4px; | |
font-size: 90%; | |
color: #c7254e; | |
background-color: #f9f2f4; | |
border-radius: 4px; | |
} |
#c7254e
isn't in our color system, and we also don't use this color for code
in /learn, so I went ahead and copied the code
styles from /learn:
code
element after the changes:
Light | Dark |
---|---|
/* Override Tailwind's default `-webkit-tap-highlight-color` rule. */ | ||
/* https://github.com/tailwindlabs/tailwindcss/discussions/2984 */ | ||
button { | ||
-webkit-tap-highlight-color: transparent; |
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.
Tailwind didn't have this rule in In earlier versions, but the rule has been added: https://github.com/tailwindlabs/tailwindcss/blob/3.4/src/css/preflight.css#L39.
// Focus state | ||
"focus:outline-none", // Hide the default browser outline | ||
"focus-visible:ring", | ||
"focus-visible:ring-focus-outline-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.
These are redundant now that the focus outline styles have been moved to the global stylesheet (base.css).
"focus:outline-none", // Hide the default browser outline | ||
"focus-visible:ring", | ||
"focus-visible:ring-focus-outline-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.
Removed as the focus outline styles are moved to the global stylesheet (base.css).
"focus:outline-none", // Hide the default browser outline | ||
"focus:ring", | ||
"focus:ring-focus-outline-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.
Removed as the focus outline styles are moved to the global stylesheet (base.css).
const classes = responsive | ||
? [className, "block max-w-full h-auto"].join(" ") |
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.
Orginally, the img
element doesn't have a max width, which allows it to expand outside of its content.
We actually do have that rule set, but I'm not sure why it doesn't take effect:
ui/src/global-element-styles.css
Lines 40 to 42 in 7f9fc26
img { | |
max-width: 100% !important; | |
} |
And in order to make the image responsive, we need to add in a couple of CSS styles:
- display: block
- max-width: 100%
- height: auto
Now, with Tailwind preflight enabled, the above rules are set by default: https://github.com/tailwindlabs/tailwindcss/blob/3.4/src/css/preflight.css#L360-L380.
And this means we have to unset the default rules (specifically the width constraint) to make the image not responsive.
(Side note: We might want to revisit the component and see if we should just make Image
always responsive. I can't think of a scenario where we want the image to scale freely.)
corePlugins: { | ||
preflight: false, | ||
}, |
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.
Enable Tailwind preflight.
(We can also just flip the boolean, but the preflight is enabled by default so I thought it's cleaner to just get rid of the code.)
/* This is required in order to improve text readability in Arabic */ | ||
text-underline-position: under; | ||
} | ||
@supports not (text-underline-position: under) { | ||
a { | ||
text-underline-offset: 0.1em; | ||
} |
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.
Pulled from normalize.css
Lines 43 to 50 in 7f9fc26
/* This is required in order to improve text readability in Arabic */ | |
text-underline-position: under; | |
} | |
@supports not (text-underline-position: under) { | |
a { | |
text-underline-offset: 1em; | |
} | |
} |
The rule is needed to ensure there is an appropriate space between the text and the underline.
17a118f
to
b13b276
Compare
/* https://github.com/tailwindlabs/tailwindcss/blob/3.4/src/css/preflight.css#L335 */ | ||
input::placeholder, | ||
textarea::placeholder { | ||
@apply text-foreground-quaternary opacity-70; |
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.
Tailwind gives the placeholder of input
and textarea
the following rules:
- opacity: 1
- color: theme('colors.gray.400', #9ca3af)
We're using foreground-quaternary
for placeholder text, which overrides the Tailwind's color:
ui/src/form-control/form-control.tsx
Line 12 in bbe314b
"outline-0 block w-full py-1.5 px-2.5 text-md text-foreground-primary placeholder:text-foreground-quaternary bg-background-primary bg-none rounded-none border-1 border-solid border-background-quaternary shadow-none transition ease-in-out duration-150 focus:border-foreground-tertiary"; |
And the color is strong enough that with opacity: 1
, the placeholder text can be mistaken with an actual input value:
To fix the issue, I had to override Tailwind color and opacity values. I also moved the placeholder rules from FormControl
to base.css
, just so that the rules can be applied globally.
(Tailwind is kind enough to set the text color for us, but we don't have gray-400
in our color list. And while the #9ca3af
fallback works, the color isn't in our design system and the contrast ratio is also insufficient, so I kept using foreground-quaternary
instead.)
Theme | Screenshot | Contrast ratio |
---|---|---|
Light | 4.53:1 (contrast checker) | |
Dark | 6.55:1 (contrast checker) |
Thanks for putting this pr together. |
Checklist:
Update index.md
)We currently have multiple stylesheets in the package, and they contain CSS rules that overlap / override each other. This PR is an attempt to consolidate them into a single stylesheet, housing the default styles of our design system.
I'll explain the changes with PR comments, but the high-level approach is:
global-element-styles.css
andnormalize.css
and pick out the styles we want to keepglobal-element-styles.css
andnormalize.css
base.css
stylesheet (focus ring, link underline position, etc.)Closes #61
Testing
I opened the production site (https://opensource.freecodecamp.org/ui) and compared the local version against it.