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

Jump to bottom

Allow conversion of rem font size to pixels in typography presets #1304

Closed
wants to merge 7 commits into from
Closed

Allow conversion of rem font size to pixels in typography presets #1304

wants to merge 7 commits into from

Conversation

jamesmockett
Copy link
Contributor

@jamesmockett jamesmockett commented Mar 25, 2024 •
edited

What are you changing?

Adds presetToPx helper to convert default rem font size in a given typography preset to a pixel value. Line height is untouched as this is already specified as a unitless relative value.

Why?

We want to encourage use of rem font sizes so rather than duplicating all of the presets with different units a conversion function provides a pixel equivalent where strictly necessary.

presetToPx(headlineBold24))

👇

font-family: "GH Guardian Headline", "Guardian Egyptian Web", Georgia, serif;
font-size: 24px;
line-height: 1.15;
font-weight: 700;
font-style: normal;
--source-text-decoration-thickness: 3px;

Notes

This adds a TypographyPreset type to define the shape of a preset, but this hasn't been applied to the presets themselves as it hides the underlying values of the properties with the expected type:

Screenshot 2024-03-27 at 16 44 16

With untyped presets the values are exposed:

Screenshot 2024-03-27 at 16 43 52

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Mar 25, 2024 •
edited

⚠️ No Changeset found

Latest commit: 7a98a77

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions github-actions bot added the 📦 npm Affects a @guardian package on NPM label Mar 25, 2024
@github-actions GitHub Actions
Copy link
Contributor

Tip

Once this PR is ready to go, add the run_chromatic label to run the Chromatic tests.

This saves us a lot of money by not running the tests before we need them.

@jamesmockett jamesmockett changed the title Jm/type preset px Allow conversion of rem font size to pixels in typography presets Mar 25, 2024
@jamesmockett jamesmockett linked an issue Mar 25, 2024 that may be closed by this pull request
Ability to get typography preset CSS with pixel values #1294
Open
@jamesmockett jamesmockett self-assigned this Mar 27, 2024
@jamesmockett jamesmockett added the run_chromatic Runs chromatic when label is applied label Mar 27, 2024
@jamesmockett jamesmockett marked this pull request as ready for review March 27, 2024 17:12
@jamesmockett jamesmockett requested review from a team as code owners March 27, 2024 17:12
mxdvl
mxdvl reviewed Mar 28, 2024
View reviewed changes
libs/@guardian/source-foundations/src/utils/typography.ts
Comment on lines +9 to +15
const matches = preset.match(REGEX_FONT_SIZE);
if (matches?.[1]) {
const pxVal = parseFloat(matches[1]) * 16;
return preset.replace(REGEX_FONT_SIZE, `font-size: ${pxVal}px`);
}

return preset;
Copy link
Member

@mxdvl mxdvl Mar 28, 2024 •
edited

Choose a reason for hiding this comment

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

You can also use a replacer function as part of String.prototype.replace, which could make this simpler:

return preset.replace(REGEX_FONT_SIZE, (_, match) => {
  const pxValue = parseFloat(match) * 16;
  return `font-size: ${pxValue}px`;
})

mxdvl
mxdvl reviewed Mar 28, 2024
View reviewed changes
libs/@guardian/source-foundations/src/utils/typography.ts
* Convert `font-size` value in typography preset from rem to px
*/
export const presetToPx = (preset: TypographyPreset) => {
const REGEX_FONT_SIZE = /font-size:\s(\d+\.\d+)rem/;
Copy link
Member

@mxdvl mxdvl Mar 28, 2024 •
edited

Choose a reason for hiding this comment

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

I think you might have an issue if the value does not contain a decimal part… Thankfully we currently do not use 16px, 32px or 48px as a font size, but we might in the future!

Suggested change
const REGEX_FONT_SIZE = /font-size:\s(\d+\.\d+)rem/;
const REGEX_FONT_SIZE = /font-size:\s(\d+\.?\d*)rem/;

Explore in Regex101

image

SiAdcock
SiAdcock reviewed Mar 28, 2024
View reviewed changes
libs/@guardian/source-foundations/src/utils/typography.ts

const matches = preset.match(REGEX_FONT_SIZE);
if (matches?.[1]) {
const pxVal = parseFloat(matches[1]) * 16;
Copy link
Contributor

Choose a reason for hiding this comment

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

This nombre magique is exposed as rootPixelFontSize in ./convert-value. Shall we pull it in here?

@jamesmockett
Copy link
Contributor Author

Thanks the comments @mxdvl and @SiAdcock 🙏 We've realised that people don't seem to be using the existing API's unit: 'px' option so given that we want to encourage the use of relative font sizes we've decided to leave this functionality out for now. We're not planning to deprecate the typography API immediately so if it turns out that there is a need for this we can resurrect the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mxdvl mxdvl mxdvl left review comments

@SiAdcock SiAdcock SiAdcock left review comments

Assignees

@jamesmockett jamesmockett

Labels
📦 npm Affects a @guardian package on NPM run_chromatic Runs chromatic when label is applied
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ability to get typography preset CSS with pixel values
3 participants
@jamesmockett @mxdvl @SiAdcock