Set all type style properties explicitly #294

joelamb · 2023-07-14T16:12:04Z

Because CSS properties such as font-weight are inherited, if a parent element sets a type style that sets a medium font weight and a child sets a style that doesn't set font weight, but is expecting normal weight, then the child will render with an unexpected medium font weight.

The same goes for text-rendering, letter-spacing and font-family, so all properties need to be explicitly set for all type styles.

changeset-bot · 2023-07-14T16:12:07Z

🦋 Changeset detected

Latest commit: 60e5d86

The changes in this PR will be included in the next version bump.

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

cloudflare-workers-and-pages · 2023-07-14T16:13:07Z

Deploying with Cloudflare Pages

Latest commit:	`60e5d86`
Status:	✅ Deploy successful!
Preview URL:	https://031c4adf.tailwind-toucan-base.pages.dev
Branch Preview URL:	https://jl-explicit-type-style-prope.tailwind-toucan-base.pages.dev

View logs

nicolechung · 2023-07-17T15:30:11Z

.changeset/spicy-mice-speak.md

+"@tailwind-toucan-base": patch
+---
+
+chore: set all type style properties explicitly


Asking @ynotdraw here: Should this actually be a BREAKING change (minor? major?) because while this is the "correct" thing to do, it also can cause visual breaking changes?

Yeah, I think a breaking change here may be safest/most valid. Especially due to:

if a parent element sets a type style that sets a medium font weight and a child sets a style that doesn't set font weight, but is expecting normal weight, then the child will render with an unexpected medium font weight.

(Also this changeset name is hilarious - spicy-mice-speak.md 🌶️ 🐭 🗣️ 😂)

changeset updated

spicy-mouse-steak 🤤

nicolechung · 2023-07-17T15:31:17Z

src/plugins/typography.js

-  let geometricPrecision = { textRendering: 'geometricPrecision' };
-  let underline = { textDecoration: 'underline' };
-
+  // geometricPrecision text rendering makes it appear slightly


Thanks for the explanation! Do we want a "this is what we want for Toucan" at the end?

I feel that's implicit since we've included it, so not sure it needs that addition.

nicolechung · 2023-07-17T15:32:19Z

src/plugins/typography.js

+      letterSpacing: theme('letterSpacing.normal'),
+      lineHeight: theme('lineHeight.8'),
+      textDecoration: 'initial',
+      textRendering: 'geometricPrecision',


Why are some of these geometricPrecision and some are initial?

oh nevermind I see that this was there from before.

nicolechung · 2023-07-17T15:41:11Z

Sorta, but not exactly related:

I read through the code but it's a bit unclear how the preview is made for font-sizes
That being said, it would be nice to have a font-styles preview because as you can see
a) font-weight is not included here with font-sizes
b) seems like the tailwind classes are not used, just a style applied to the font-size

ynotdraw · 2023-07-17T15:46:19Z

package.json

@@ -82,6 +82,7 @@
  "volta": {
    "node": "18.16.1",
    "yarn": "1.22.19",
-    "npm": "9.8.0"
+    "npm": "9.8.0",
+    "pnpm": "7.32.5"


I think we can probably have only node and pnpm under here, as it looks like we aren't using yarn or npm

ynotdraw · 2023-07-17T15:47:45Z

.changeset/spicy-mice-speak.md

+"@tailwind-toucan-base": patch
+---
+
+chore: set all type style properties explicitly


Yeah, I think a breaking change here may be safest/most valid. Especially due to:

if a parent element sets a type style that sets a medium font weight and a child sets a style that doesn't set font weight, but is expecting normal weight, then the child will render with an unexpected medium font weight.

(Also this changeset name is hilarious - spicy-mice-speak.md 🌶️ 🐭 🗣️ 😂)

pin [email protected] in line with other CS repositories

ynotdraw

🎉

clintcs · 2023-07-24T13:16:52Z

src/plugins/typography.js

@@ -8,13 +8,11 @@ module.exports = (fontSizes) =>
  });



A comment explaining why certain properties are set to initial would be helpful, I think. Readers will otherwise spend a second wondering before assuming.

Also, not a big deal, but why initial instead of the desired value? The effect is the same. But setting, say, text-rendering to auto is a more explicit and saves the reader a moment's time.

I went with initial over auto to convey the concept that this is not setting a value, but would have the effect of 'un-setting' any inherited value. Will add a comment to that effect, but leave it as initial rather than auto

Because CSS properties such as font-weight are inherited, if a parent element sets a type style that sets a medium font weight and a child sets a style that doesn't set font weight, but is expecting normal weight, then the child will render with an unexpected medium font weight. The same goes for text-rendering, letter-spacing and font-family, so all properties need to be explicitly set for all type styles.

joelamb force-pushed the jl-explicit-type-style-properties branch 3 times, most recently from 04df5dc to e3341cf Compare July 17, 2023 14:56

joelamb requested review from nicolechung and ynotdraw July 17, 2023 14:59

nicolechung reviewed Jul 17, 2023

View reviewed changes

ynotdraw reviewed Jul 17, 2023

View reviewed changes

chore: pin pnpm version

7a12769

pin [email protected] in line with other CS repositories

joelamb force-pushed the jl-explicit-type-style-properties branch from b5d68dc to 40cd1d4 Compare July 24, 2023 09:41

joelamb requested review from nicolechung and ynotdraw July 24, 2023 10:26

joelamb changed the title ~~chore: set all type style properties explicitly~~ Set all type style properties explicitly Jul 24, 2023

joelamb requested a review from clintcs July 24, 2023 12:35

ynotdraw approved these changes Jul 24, 2023

View reviewed changes

joelamb force-pushed the jl-explicit-type-style-properties branch from dcae615 to f83f092 Compare July 24, 2023 12:48

clintcs reviewed Jul 24, 2023

View reviewed changes

clintcs approved these changes Jul 24, 2023

View reviewed changes

joelamb added 3 commits July 24, 2023 16:11

chore: update snapshots

37e1313

chore: create changelog

60e5d86

joelamb force-pushed the jl-explicit-type-style-properties branch from f83f092 to 60e5d86 Compare July 24, 2023 15:12

joelamb merged commit f9f53e5 into main Jul 24, 2023

joelamb deleted the jl-explicit-type-style-properties branch July 24, 2023 15:41

github-actions bot mentioned this pull request Jul 25, 2023

Release Preview #260

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set all type style properties explicitly #294

Set all type style properties explicitly #294

joelamb commented Jul 14, 2023

changeset-bot bot commented Jul 14, 2023 •

edited

Loading

cloudflare-workers-and-pages bot commented Jul 14, 2023 •

edited

Loading

nicolechung Jul 17, 2023

ynotdraw Jul 17, 2023

joelamb Jul 24, 2023

clintcs Jul 24, 2023

nicolechung Jul 17, 2023

joelamb Jul 24, 2023

nicolechung Jul 17, 2023

nicolechung Jul 17, 2023

nicolechung commented Jul 17, 2023

ynotdraw Jul 17, 2023 •

edited

Loading

joelamb Jul 24, 2023

ynotdraw Jul 17, 2023

ynotdraw left a comment

clintcs Jul 24, 2023 •

edited

Loading

joelamb Jul 24, 2023 •

edited

Loading

Set all type style properties explicitly #294

Set all type style properties explicitly #294

Conversation

joelamb commented Jul 14, 2023

changeset-bot bot commented Jul 14, 2023 • edited Loading

🦋 Changeset detected

cloudflare-workers-and-pages bot commented Jul 14, 2023 • edited Loading

Deploying with Cloudflare Pages

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

nicolechung commented Jul 17, 2023

ynotdraw Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ynotdraw left a comment

Choose a reason for hiding this comment

clintcs Jul 24, 2023 • edited Loading

Choose a reason for hiding this comment

joelamb Jul 24, 2023 • edited Loading

Choose a reason for hiding this comment

changeset-bot bot commented Jul 14, 2023 •

edited

Loading

cloudflare-workers-and-pages bot commented Jul 14, 2023 •

edited

Loading

ynotdraw Jul 17, 2023 •

edited

Loading

clintcs Jul 24, 2023 •

edited

Loading

joelamb Jul 24, 2023 •

edited

Loading