refactor: consolidate CSS stylesheets #114

huyenltnguyen · 2024-05-03T19:09:33Z

Checklist:

I have read freeCodeCamp's contribution guidelines.
My pull request has a descriptive title (not a vague title like 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:

Enable Tailwind preflight
Review the styles in global-element-styles.css and normalize.css and pick out the styles we want to keep
Remove global-element-styles.css and normalize.css
Add additional global styles to the base.css stylesheet (focus ring, link underline position, etc.)
Check and fix UI regression

Closes #61

Testing

I opened the production site (https://opensource.freecodecamp.org/ui) and compared the local version against it.

huyenltnguyen · 2024-05-07T07:47:06Z

src/base.css

-		*/
-		border-width: 0;
+	html {
+		@apply text-md;


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:

ui/tailwind.config.js

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 the sans font: https://github.com/tailwindlabs/tailwindcss/blob/f1f419a9ecfcd00a2001ee96ab252739fca47564/src/css/preflight.css#L36

code, kbd, samp, and pre get the mono font: https://github.com/tailwindlabs/tailwindcss/blob/f1f419a9ecfcd00a2001ee96ab252739fca47564/src/css/preflight.css#L115

sans and mono fonts are specified here:

ui/tailwind.config.js

Lines 90 to 93 in 7f9fc26

fontFamily: {

sans: ["Lato", "sans-serif"],

mono: ["Hack-ZeroSlash", "monospace"],

},

huyenltnguyen · 2024-05-07T07:49:25Z

src/base.css

-		/* Override the browser default border width in order to style individual border sides
-		* Ref: https://stackoverflow.com/a/76961084
-		*/
-		border-width: 0;


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

huyenltnguyen · 2024-05-07T07:52:09Z

src/base.css

+	:focus-visible {
+		@apply outline outline-3 outline-focus-outline-color outline-offset-0;
+	}


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.

huyenltnguyen · 2024-05-07T08:06:31Z

src/base.css

+	code {
+		@apply bg-background-tertiary text-foreground-tertiary;
+	}
+	:not(pre) > code {
+		@apply border-1 border-gray-450;
+	}


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:

https://github.com/freeCodeCamp/freeCodeCamp/blob/5694b0837c7bad757777d15b9f196a1d8d3c8dae/client/src/components/layouts/global.css#L458-L465

code element after the changes:

Light Dark

huyenltnguyen · 2024-05-07T08:10:44Z

src/base.css

-	/* Override Tailwind's default `-webkit-tap-highlight-color` rule. */
-	/* https://github.com/tailwindlabs/tailwindcss/discussions/2984 */
-	button {
-		-webkit-tap-highlight-color: transparent;


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.

huyenltnguyen · 2024-05-07T08:34:36Z

src/button/button.tsx

-	// Focus state
-	"focus:outline-none", // Hide the default browser outline
-	"focus-visible:ring",
-	"focus-visible:ring-focus-outline-color",


These are redundant now that the focus outline styles have been moved to the global stylesheet (base.css).

huyenltnguyen · 2024-05-07T08:34:44Z

src/close-button/close-button.tsx

-		"focus:outline-none", // Hide the default browser outline
-		"focus-visible:ring",
-		"focus-visible:ring-focus-outline-color",


Removed as the focus outline styles are moved to the global stylesheet (base.css).

huyenltnguyen · 2024-05-07T08:35:37Z

src/toggle-button/toggle-button.tsx

-	"focus:outline-none", // Hide the default browser outline
-	"focus:ring",
-	"focus:ring-focus-outline-color",


Removed as the focus outline styles are moved to the global stylesheet (base.css).

huyenltnguyen · 2024-05-07T08:44:53Z

src/image/image.tsx

 		const classes = responsive
-			? [className, "block max-w-full h-auto"].join(" ")


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.)

huyenltnguyen · 2024-05-07T08:50:40Z

tailwind.config.js

-	corePlugins: {
-		preflight: false,
-	},


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.)

huyenltnguyen · 2024-05-07T15:16:46Z

src/base.css

+		/* 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;
+		}


Pulled from normalize.css

ui/src/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.

Ref: freeCodeCamp/freeCodeCamp#52366.

…nsolidate-css-stylesheets

huyenltnguyen · 2024-05-08T06:55:17Z

src/base.css

+	/* https://github.com/tailwindlabs/tailwindcss/blob/3.4/src/css/preflight.css#L335 */
+	input::placeholder,
+	textarea::placeholder {
+		@apply text-foreground-quaternary opacity-70;


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)

ahmaxed · 2024-05-18T07:35:25Z

Thanks for putting this pr together.
Once bootstrap is removed, we can gradually remove some of our customizations in favor of best practices or defaults.

huyenltnguyen added 8 commits May 3, 2024 12:48

feat: enable Tailwind preflight

d29e79d

chore: remove redundant styles

5705599

feat: add custom rules to base.css

24f5486

chore: remove extra stylesheets from Storybook preview

4c81a65

fix: a bunch of things

55ceb8d

chore: remove normalize.css and global-element-styles.css

57e128e

replace fixed value with tailwind style

a6cc403

heading bottom margin

a74212d

huyenltnguyen commented May 7, 2024

View reviewed changes

use em for link underline

1f27906

huyenltnguyen commented May 7, 2024

View reviewed changes

huyenltnguyen added 3 commits May 7, 2024 23:37

override Tailwind placeholder text color

5da1543

Merge branch 'main' of https://github.com/freeCodeCamp/ui into fix/co…

ad6cb21

…nsolidate-css-stylesheets

improve placeholder text contrast

b13b276

huyenltnguyen force-pushed the fix/consolidate-css-stylesheets branch from 17a118f to b13b276 Compare May 8, 2024 06:42

huyenltnguyen commented May 8, 2024

View reviewed changes

huyenltnguyen marked this pull request as ready for review May 8, 2024 06:56

huyenltnguyen requested a review from a team as a code owner May 8, 2024 06:56

huyenltnguyen assigned ahmaxed May 8, 2024

huyenltnguyen mentioned this pull request May 8, 2024

Storybook doesn't apply the correct font family #121

Closed

ahmaxed merged commit fc7654f into freeCodeCamp:main May 18, 2024
5 checks passed

huyenltnguyen deleted the fix/consolidate-css-stylesheets branch May 20, 2024 05:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: consolidate CSS stylesheets #114

refactor: consolidate CSS stylesheets #114

huyenltnguyen commented May 3, 2024 •

edited

huyenltnguyen May 7, 2024

huyenltnguyen May 7, 2024

huyenltnguyen May 7, 2024

huyenltnguyen May 7, 2024

huyenltnguyen May 7, 2024

huyenltnguyen May 7, 2024

huyenltnguyen May 7, 2024

huyenltnguyen May 7, 2024

huyenltnguyen May 7, 2024

huyenltnguyen May 7, 2024

huyenltnguyen May 7, 2024

huyenltnguyen May 8, 2024

ahmaxed commented May 18, 2024

	/* Element styles rewrites from global.css that could be moved to presets*/
	/* typography should be handled in tailwind config */

	fontFamily: {
	sans: ["Lato", "sans-serif"],
	mono: ["Hack-ZeroSlash", "monospace"],
	},

	code {
	padding: 2px 4px;
	font-size: 90%;
	color: #c7254e;
	background-color: #f9f2f4;
	border-radius: 4px;
	}

		const classes = responsive
		? [className, "block max-w-full h-auto"].join(" ")

	/* 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;
	}
	}

refactor: consolidate CSS stylesheets #114

refactor: consolidate CSS stylesheets #114

Conversation

huyenltnguyen commented May 3, 2024 • edited

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmaxed commented May 18, 2024

huyenltnguyen commented May 3, 2024 •

edited