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.

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

Use relative fonts + spacing in Live Blog Posts #646

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

CLTPayne
Copy link
Contributor

@CLTPayne CLTPayne commented Feb 8, 2022

Switch from px to rems in Live Blog Posts (tentative as per below)

Context:

  1. In the app we're doing some work to see how far we can move to relative units for fonts and some spacing so that ultimately we can give the user the option toggling the font size app wide.
  2. Origami by default outputs px units but has these handy flags to output rem units - https://github.com/Financial-Times/origami/blob/813c8e46d9e6e423fad3a1322d043edc6422cd69/components/o-typography/src/scss/_variables.scss#L5-L12.
  3. I explored overriding these styles on the App side only but found that we're consuming the bundled CSS so the styles are all already outputted in px (so we can't override the default with the origami flags). We can override the individual element styles to all be rem but there is quite a lot of them so this feels very brittle and verbose. Thus I'm suggesting we the change as per this PR. (Note there is work in progress to change this consumption of bundled CSS but it is a few weeks away minimum).
  4. Doing the change here in the x-dash component also means we benefit FT.com users who might modify their font size using browser settings.

In this PR:

Override oSpacing and oTypography in the live blog post to output rem units. This PR is raised as a discussion to get better understanding of the FT.com side... and the 0.8125em base font-size.

Testing this change in the App:

With default font size selected in the app

Before After

With extra large font size selected:

Before After

Testing this change on FT.com / next-article:

This is potentially an issue because I'm reducing the 'medium' font-size... I'm not sure though as it seems odd to me (but I know nothing about ft.com and haven't managed to find a documented reason) that we set the base font-size to be 0.8125em - https://github.com/Financial-Times/n-ui-foundations/blob/8261b9c7565bd49e5988da3f7b99db64c859cc88/typography/main.scss#L19 (via dotcom-ui-basestyles component https://github.com/Financial-Times/dotcom-page-kit/blob/main/packages/dotcom-ui-base-styles/styles.scss#L10).

With default / 'medium' font size selected in Chrome browser settings:

Before After
Computed font-size is 18px on the blog post text (due to px origami outputs) Computed font-size is 14.625px (due to 0.8125em base font-size)

With 'very large' font size selected in Chrome browser settings:

Before After
Computed font-size is 18px on the blog post text (due to px origami outputs) Computed font-size is 21.9375px (due to 0.8125em base font-size)

* For legacy reasons, Origami output typography and spacing in px by
default. They provide these flags to over ride this and out put rems.
* When `true`, the user will be able to modify their font size using
browser settings on FT.com
* Reference:
https://github.com/Financial-Times/origami/blob/813c8e46d9e6e423fad3a1322d043edc6422cd69/components/o-typography/src/scss/_variables.scss#L5-L12
@CLTPayne CLTPayne requested a review from a team as a code owner February 8, 2022 17:49
@next-team next-team temporarily deployed to x-dash-at-5052-responsi-610gkz February 8, 2022 17:49 Inactive
@CLTPayne
Copy link
Contributor Author

CLTPayne commented Feb 9, 2022

Hmmm I've also just noticed that on the new front-page we're actively removing this base font-size: https://github.com/Financial-Times/next-home-page/blob/586c10fd5fddd2546f586ee8153cca189959c2a0/apps/home-page/client/main.scss#L26-L28

Doing the same on the article page would give us the behaviour I expected and a computed font-size of 27px if a user changes their browser font setting to 'very large'.

@notlee
Copy link
Contributor

notlee commented Feb 9, 2022

I'm curious why the root font size is 0.8125em too? We should probably explicitly set 16px unless we are able to use relative units consistently (much preferable).

Some Origami components have alignment issues when:

  1. the relative unit variables aren't set to true; and
  2. 16px is not the base font size

See o-message screenshot:
Financial-Times/origami#195

@CLTPayne
Copy link
Contributor Author

Update - FT.com hive mind (or just Dawn) clarified that this 0.8125em is a legacy setting because old browsers (that we used to support) did not cope with a base setting of px and relative units elsewhere. https://financialtimes.slack.com/archives/C041V9QA7/p1644503777253009. We no longer support this old behaviour, but changing this across FT.com is thought to need a lot of testing to make sure there are no bugs introduced across the different apps. Hence the new front page moving away from this 0.8125em.

Soooooo, as Content Innovation / Story Telling are also the code owners of next-article, I'm wondering what the team thinks about exploring unsetting the 0.8125em in next-article. Is this feasible to test? Or should I :retreat: and stop poking this bear?

@notlee
Copy link
Contributor

notlee commented Feb 11, 2022

I see, thanks for the context @CLTPayne! I'd say let's poke this bear because0.8125em is currently causing layout bugs on ft.com and will cause issues for any user who has configured their browser's font size. I just had a quick look on the article page, setting 16px on the root, and it only had a minor affect on some margins – as well as improving o-messaging layout.

In the long term ft.com should work toward respecting user text size preference with relative units but since we use absolute values in most places that's not happening, so swapping 0.8125em for 16px until then is sensible

@GlynnPhillips
Copy link
Contributor

I think that changing the root font size is likely going to need to be a bit of a project on its own so we are prepared to react to any issues that might arise from that. The storytelling team are in no position to deal with this at the moment (just lost 2 engineers).

I think we should probably prioritise short term solutions so Charlotte can move forward with whats important to her team which will still be moving this component in the right direction and allow FT.com to move in its own time. The options I would suggest are

  1. Make this change and release it as a new breaking change, that way ft.com can avoid using it for now
  2. Create a new breaking change that allows for both options (absolute and relative) based on an option thats passed in to the component

The latter is probably more work and would result in some duplicate styles (very minimal though) but it would help prevent a future forced upgrade when new features are wanted in live blogs and we can't do it until we which to relative (or backwards port any changes).

@GlynnPhillips
Copy link
Contributor

Not sure if this has been raised anywhere but if FT.com moves towards better supporting relative font sizes we would probably want to support grid changes at the same time (I don't think we do this yet???)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants