Skip to content
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.

Sign up for GitHub

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

Set all type style properties explicitly #294

Merged
merged 4 commits into from
Jul 24, 2023
Merged

Set all type style properties explicitly #294

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7a12769
chore: pin pnpm version
joelamb Jul 17, 2023
5ba46dc
chore: set all type style properties explicitly
joelamb Jul 14, 2023
37e1313
chore: update snapshots
joelamb Jul 17, 2023
60e5d86
chore: create changelog
joelamb Jul 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/spicy-mice-speak.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"@tailwind-toucan-base": major
---

possibly breaking change that sets all type style properties explicitly,
which could cause subtle, but unexpected, changes in the weight of rendered
text if inherited parent styles are no longer applied.
3 changes: 1 addition & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,6 @@
"packageManager": "[email protected]",
"volta": {
"node": "18.17.0",
"yarn": "1.22.19",
"npm": "9.8.1"
"pnpm": "7.32.5"
Copy link
Contributor

@ynotdraw ynotdraw Jul 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

}
}
134 changes: 109 additions & 25 deletions src/plugins/typography.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,11 @@ module.exports = (fontSizes) =>
});

Copy link

@clintcs clintcs Jul 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@joelamb joelamb Jul 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

function addTextStyles({ addComponents, theme }, fontSize) {
// https://developer.mozilla.org/en-US/docs/Web/CSS/text-rendering
let geometricPrecision = { textRendering: 'geometricPrecision' };
let underline = { textDecoration: 'underline' };

// geometricPrecision text rendering makes it appear slightly
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// lighter than the standard font-weight
let text = {
'.geometric-precision-text': {
...geometricPrecision,
textRendering: 'geometricPrecision',
},
};

Expand All @@ -24,114 +22,200 @@ function addTextStyles({ addComponents, theme }, fontSize) {
* fontSize and lineHeight when used in this type of object, every usage
* provides *two* fontSizes (one desired, one is supposed to be the lineHeight,
* but is interpreted as a fontSize here).
*
* We use the `initial` value to unset values that might be otherwise
* inherited from parent elements eg. textRendering, textDecoration
*/
let textStyles = {
'.type-4xl': {
fontFamily: theme('fontFamily.sans'),
fontSize: fontSize['4xl'],
lineHeight: theme('lineHeight.13'),
fontWeight: theme('fontWeight.medium'),
letterSpacing: theme('letterSpacing.tight'),
lineHeight: theme('lineHeight.13'),
textDecoration: 'initial',
textRendering: 'initial',
},
'.type-3xl': {
fontFamily: theme('fontFamily.sans'),
fontSize: fontSize['3xl'],
lineHeight: theme('lineHeight.12'),
fontWeight: theme('fontWeight.medium'),
letterSpacing: theme('letterSpacing.tight'),
lineHeight: theme('lineHeight.12'),
textDecoration: 'initial',
textRendering: 'initial',
},
'.type-2xl': {
fontFamily: theme('fontFamily.sans'),
fontSize: fontSize['2xl'],
lineHeight: theme('lineHeight.10'),
fontWeight: theme('fontWeight.medium'),
letterSpacing: theme('letterSpacing.normal'),
lineHeight: theme('lineHeight.10'),
textDecoration: 'initial',
textRendering: 'initial',
},
'.type-xl': {
fontFamily: theme('fontFamily.sans'),
fontSize: fontSize.xl,
lineHeight: theme('lineHeight.8'),
fontWeight: theme('fontWeight.medium'),
...geometricPrecision,
letterSpacing: theme('letterSpacing.normal'),
lineHeight: theme('lineHeight.8'),
textDecoration: 'initial',
textRendering: 'geometricPrecision',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are some of these geometricPrecision and some are initial?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh nevermind I see that this was there from before.

},
'.type-lg': {
fontFamily: theme('fontFamily.sans'),
fontSize: fontSize.lg,
fontWeight: theme('fontWeight.normal'),
letterSpacing: theme('letterSpacing.normal'),
lineHeight: theme('lineHeight.8'),
textDecoration: 'initial',
textRendering: 'initial',
},
'.type-lg-medium': {
fontFamily: theme('fontFamily.sans'),
fontSize: fontSize.lg,
lineHeight: theme('lineHeight.8'),
fontWeight: theme('fontWeight.medium'),
letterSpacing: theme('letterSpacing.normal'),
lineHeight: theme('lineHeight.8'),
textDecoration: 'initial',
textRendering: 'initial',
},
'.type-lg-tight-medium': {
fontFamily: theme('fontFamily.sans'),
fontSize: fontSize.lg,
lineHeight: theme('lineHeight.6'),
fontWeight: theme('fontWeight.medium'),
...geometricPrecision,
letterSpacing: theme('letterSpacing.normal'),
lineHeight: theme('lineHeight.6'),
textDecoration: 'initial',
textRendering: 'geometricPrecision',
},
'.type-md': {
fontFamily: theme('fontFamily.sans'),
fontSize: fontSize.md,
fontWeight: theme('fontWeight.normal'),
letterSpacing: theme('letterSpacing.normal'),
lineHeight: theme('lineHeight.6'),
textDecoration: 'initial',
textRendering: 'initial',
},
// deprecated? md-medium?
'.type-md-medium': {
fontFamily: theme('fontFamily.sans'),
fontSize: fontSize.md,
lineHeight: theme('lineHeight.6'),
fontWeight: theme('fontWeight.medium'),
letterSpacing: theme('letterSpacing.normal'),
lineHeight: theme('lineHeight.6'),
textDecoration: 'initial',
textRendering: 'initial',
},
'.type-md-underline': {
fontFamily: theme('fontFamily.sans'),
fontSize: fontSize.md,
fontWeight: theme('fontWeight.normal'),
letterSpacing: theme('letterSpacing.normal'),
lineHeight: theme('lineHeight.6'),
...underline,
textDecoration: 'underline',
textRendering: 'initial',
},
'.type-md-tight': {
fontFamily: theme('fontFamily.sans'),
fontSize: fontSize.md,
fontWeight: theme('fontWeight.normal'),
letterSpacing: theme('letterSpacing.normal'),
lineHeight: theme('lineHeight.5'),
textDecoration: 'initial',
textRendering: 'initial',
},
'.type-md-tight-medium': {
fontFamily: theme('fontFamily.sans'),
fontSize: fontSize.md,
lineHeight: theme('lineHeight.5'),
fontWeight: theme('fontWeight.medium'),
...geometricPrecision,
letterSpacing: theme('letterSpacing.normal'),
lineHeight: theme('lineHeight.5'),
textDecoration: 'initial',
textRendering: 'geometricPrecision',
},
'.type-md-tight-underline': {
fontFamily: theme('fontFamily.sans'),
fontSize: fontSize.md,
fontWeight: theme('fontWeight.normal'),
letterSpacing: theme('letterSpacing.normal'),
lineHeight: theme('lineHeight.5'),
...underline,
textDecoration: 'underline',
textRendering: 'initial',
},
'.type-sm-mono': {
fontFamily: theme('fontFamily.mono'),
fontSize: fontSize.sm,
fontWeight: theme('fontWeight.normal'),
letterSpacing: theme('letterSpacing.normal'),
lineHeight: theme('lineHeight.6'),
fontFamily: theme('fontFamily.mono'),
textDecoration: 'initial',
textRendering: 'initial',
},
'.type-sm-mono-tight': {
fontSize: fontSize.sm,
fontFamily: theme('fontFamily.mono'),
fontSize: fontSize.sm,
fontWeight: theme('fontWeight.normal'),
letterSpacing: theme('letterSpacing.normal'),
lineHeight: theme('lineHeight.5'),
textDecoration: 'initial',
textRendering: 'initial',
},
'.type-sm-mono-underline': {
fontFamily: theme('fontFamily.mono'),
fontSize: fontSize.sm,
fontWeight: theme('fontWeight.normal'),
letterSpacing: theme('letterSpacing.normal'),
lineHeight: theme('lineHeight.6'),
fontFamily: theme('fontFamily.mono'),
...underline,
textDecoration: 'underline',
textRendering: 'initial',
},
'.type-xs': {
fontFamily: theme('fontFamily.sans'),
fontSize: fontSize.xs,
fontWeight: theme('fontWeight.normal'),
letterSpacing: theme('letterSpacing.normal'),
lineHeight: theme('lineHeight.5'),
textDecoration: 'initial',
textRendering: 'initial',
},
'.type-xs-tight': {
fontFamily: theme('fontFamily.sans'),
fontSize: fontSize.xs,
fontWeight: theme('fontWeight.normal'),
letterSpacing: theme('letterSpacing.normal'),
lineHeight: theme('lineHeight.4'),
textDecoration: 'initial',
textRendering: 'initial',
},
'.type-xs-tight-underline': {
fontFamily: theme('fontFamily.sans'),
fontSize: fontSize.xs,
fontWeight: theme('fontWeight.normal'),
letterSpacing: theme('letterSpacing.normal'),
lineHeight: theme('lineHeight.4'),
...underline,
textDecoration: 'underline',
textRendering: 'initial',
},
'.type-xs-mono': {
fontFamily: theme('fontFamily.mono'),
fontSize: fontSize.xs,
fontWeight: theme('fontWeight.normal'),
letterSpacing: theme('letterSpacing.normal'),
lineHeight: theme('lineHeight.6'),
fontFamily: theme('fontFamily.mono'),
textDecoration: 'initial',
textRendering: 'initial',
},
'.type-xxs-mono': {
fontFamily: theme('fontFamily.mono'),
fontSize: fontSize.xxs,
fontWeight: theme('fontWeight.normal'),
letterSpacing: theme('letterSpacing.normal'),
lineHeight: theme('lineHeight.4'),
fontFamily: theme('fontFamily.mono'),
textDecoration: 'initial',
textRendering: 'initial',
},
};

Expand Down
Loading