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

feat: use typography tokens in o-topper and o-teaser #1938

Merged
merged 30 commits into from
Jan 31, 2025

Conversation

frshwtr
Copy link
Contributor

@frshwtr frshwtr commented Jan 20, 2025

Describe your changes

Uses typography tokens within the o-topper component

Issue ticket number and link

Link to Figma designs

Checklist before requesting a review

  • I have applied percy label for o-[COMPONENT] or chromatic label for o3-[COMPONENT] on my PR before merging and after review. Find more details in CONTRIBUTING.md
  • If it is a new feature, I have added thorough tests.
  • I have updated relevant docs.
  • I have updated relevant env variables in Doppler.

@frshwtr
Copy link
Contributor Author

frshwtr commented Jan 20, 2025

Todo: cross check topper components

@frshwtr
Copy link
Contributor Author

frshwtr commented Jan 20, 2025

Todo: there are some other private typography uses e.g oPrivateTypographyLink and one for UL too, make sure to change these.

@frshwtr frshwtr changed the title feat: use typography tokens in o-topper feat: use typography tokens in o-topper and o-teaser Jan 21, 2025
@frshwtr frshwtr force-pushed the 2025-release-use-typography-tokens-in-topper branch from 7dbc994 to 4f39f1d Compare January 21, 2025 11:55
@frshwtr frshwtr marked this pull request as ready for review January 21, 2025 15:29
@frshwtr frshwtr requested a review from a team as a code owner January 21, 2025 15:29
@frshwtr
Copy link
Contributor Author

frshwtr commented Jan 21, 2025

Increased font/line height is making the background of this text container larger
This branch
image
Main
image

@frshwtr frshwtr force-pushed the 2025-release-use-typography-tokens-in-topper branch from facabe1 to d9c80c1 Compare January 21, 2025 16:08
Copy link
Contributor

@notlee notlee left a comment

Choose a reason for hiding this comment

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

Lots here! I'm not convinced it's all related to this PR though...

Comment on lines 40 to 43
font-family: oPrivateFoundationGet('o3-typography-use-case-display-sm-font-family');
font-size: oPrivateFoundationGet('o3-typography-use-case-display-sm-font-size');
font-weight: 400;
line-height: oPrivateFoundationGet('o3-typography-use-case-display-sm-line-height');
Copy link
Contributor

Choose a reason for hiding this comment

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

Screenshot 2025-01-21 at 16 23 21

This isn't using the usecase properly as it's a custom font weight (400 is also too high compared with Metric is seems). I think headline-md fits better here (please double check)?

Suggested change
font-family: oPrivateFoundationGet('o3-typography-use-case-display-sm-font-family');
font-size: oPrivateFoundationGet('o3-typography-use-case-display-sm-font-size');
font-weight: 400;
line-height: oPrivateFoundationGet('o3-typography-use-case-display-sm-line-height');
font-family: oPrivateFoundationGet('o3-typography-use-case-headline-md-font-family');
font-size: oPrivateFoundationGet('o3-typography-use-case-headline-md-font-size');
font-weight: oPrivateFoundationGet('o3-typography-use-case-headline-md-line-height');
line-height: oPrivateFoundationGet('o3-typography-use-case-headline-md-line-height');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -103,7 +118,10 @@
}

@mixin _oTopperTags {
@include oPrivateTypographySans(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

We've lost the colour of the topic tags somehow? An issue with this PR or..?

Screenshot 2025-01-21 at 16 23 21
Screenshot 2025-01-21 at 16 23 56

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems to be related to the private foundation colour changes we did a few sprints ago. Will take a look.

Comment on lines 56 to 58
color: oPrivateFoundationGet('o3-color-palette-white');
position: absolute;
text-shadow: 1px 1px 1px oPrivateFoundationGet('o3-color-palette-slate');
Copy link
Contributor

Choose a reason for hiding this comment

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

Comparing the latest release on npm (demo via the build service). The image credit no longer appears white with a shadow, it's along with the caption instead. I don't see an obvious change in this PR to explain it. What happened here?

Screenshot 2025-01-21 at 16 40 06

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For me they display identical, though I see other issues. Can we catch up some point this week to see why it looks differently for both of us?
image

components/o-topper/src/scss/_elements.scss Outdated Show resolved Hide resolved
components/o-topper/src/scss/themes/_branded.scss Outdated Show resolved Hide resolved
@@ -8,7 +8,10 @@
}

Copy link
Contributor

Choose a reason for hiding this comment

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

_oTopperColumnistName? 😱
Screenshot 2025-01-21 at 16 52 19

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 believe most of the colour issues are unrelated to the typography changes. I am happy to fix on this PR or move to a ticket. Either way it will need fixing this sprint!

Comment on lines 9 to 17
font-family: oPrivateFoundationGet('o3-typography-use-case-display-sm-font-family');
font-size: oPrivateFoundationGet('o3-typography-use-case-display-sm-font-size');
font-weight: oPrivateFoundationGet('o3-typography-use-case-display-sm-font-weight');
line-height: oPrivateFoundationGet('o3-typography-use-case-display-sm-line-height');
@include oPrivateGridRespondTo('L') {
@include oPrivateTypographyDisplay(7);
font-family: oPrivateFoundationGet('o3-typography-use-case-display-lg-font-family');
font-size: oPrivateFoundationGet('o3-typography-use-case-display-lg-font-size');
font-weight: oPrivateFoundationGet('o3-typography-use-case-display-lg-font-weight');
line-height: oPrivateFoundationGet('o3-typography-use-case-display-lg-line-height');
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you've missed some oPrivateTypographyDisplay calls in o-teaser. Looks like the following screenshot is a little too bold, maybe title would be better? Think it's coming from o-teaser__heading in _oTeaserLarge.
Screenshot 2025-01-21 at 17 00 46

Copy link
Contributor

Choose a reason for hiding this comment

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

A similar example here in demo core-large-opinion. But also, why does the copy colour now match the background? 😱
Screenshot 2025-01-21 at 17 12 12

@@ -4,7 +4,6 @@

@import 'src/scss/variables';
@import 'src/scss/mixins';
Copy link
Contributor

Choose a reason for hiding this comment

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

_oTeaserTimestamp is tiny! Let's remove its use of oLabelsTimestampContent and set to label
Screenshot 2025-01-21 at 17 10 25

@notlee notlee temporarily deployed to origami-webs-2025-relea-xdkicd January 22, 2025 09:49 Inactive
@notlee notlee temporarily deployed to origami-webs-2025-relea-xdkicd January 22, 2025 09:52 Inactive
@notlee notlee temporarily deployed to origami-webs-2025-relea-xdkicd January 22, 2025 13:27 Inactive
Comment on lines 160 to 171
@include oEditorialTypographyHeading($level: 1);
font-family: oPrivateFoundationGet('o3-typography-use-case-headline-md-font-family');
font-size: oPrivateFoundationGet('o3-typography-use-case-headline-md-font-size');
font-weight: oPrivateFoundationGet('o3-typography-use-case-headline-md-font-weight');
line-height: oPrivateFoundationGet('o3-typography-use-case-headline-md-line-height');
margin-bottom: oPrivateSpacingByIncrement(5);
}

.o-topper__headline--large {
@include oEditorialTypographyHeadline();
font-family: oPrivateFoundationGet('o3-typography-use-case-headline-lg-font-family');
font-size: oPrivateFoundationGet('o3-typography-use-case-headline-lg-font-size');
font-weight: oPrivateFoundationGet('o3-typography-use-case-headline-lg-font-weight');
line-height: oPrivateFoundationGet('o3-typography-use-case-headline-lg-line-height');
Copy link
Contributor

Choose a reason for hiding this comment

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

oEditorialTypographyHeading(1) / oEditorialTypographyHeadline were responsive before right? This should recreate that using tokens. o-topper__headline should match the headline o3-editorial-typography component and o-topper__headline--large should match the display component, both change the type usecase size responsively.

Copy link
Contributor

@notlee notlee Jan 22, 2025

Choose a reason for hiding this comment

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

Something like

		font-family: oPrivateFoundationGet('o3-typography-use-case-headline-sm-font-family');
		font-size: oPrivateFoundationGet('o3-typography-use-case-headline-sm-font-size');
		font-weight: oPrivateFoundationGet('o3-typography-use-case-headline-sm-font-weight');
		line-height: oPrivateFoundationGet('o3-typography-use-case-headline-sm-line-height');

		@include oPrivateGridRespondTo('S') {
			font-family: oPrivateFoundationGet('o3-typography-use-case-headline-md-font-family');
			font-size: oPrivateFoundationGet('o3-typography-use-case-headline-md-font-size');
			font-weight: oPrivateFoundationGet('o3-typography-use-case-headline-md-font-weight');
			line-height: oPrivateFoundationGet('o3-typography-use-case-headline-md-line-height');
		}

		@include oPrivateGridRespondTo('L') {
			font-family: oPrivateFoundationGet('o3-typography-use-case-headline-lg-font-family');
			font-size: oPrivateFoundationGet('o3-typography-use-case-headline-lg-font-size');
			font-weight: oPrivateFoundationGet('o3-typography-use-case-headline-lg-font-weight');
			line-height: oPrivateFoundationGet('o3-typography-use-case-headline-lg-line-height');
		}

@notlee notlee temporarily deployed to origami-webs-2025-relea-xdkicd January 22, 2025 15:42 Inactive
@notlee notlee temporarily deployed to origami-webs-2025-relea-xdkicd January 22, 2025 15:57 Inactive
@notlee notlee temporarily deployed to origami-webs-2025-relea-xdkicd January 23, 2025 11:23 Inactive
@notlee notlee temporarily deployed to origami-webs-2025-relea-xdkicd January 23, 2025 14:17 Inactive
@notlee
Copy link
Contributor

notlee commented Jan 27, 2025

There's an issue with the font on o-topper__columnist-name and the colour/weight of the opinion variant of o-topper__topic, otherwise the topper all looks good now 👍
Screenshot 2025-01-27 at 17 27 43

@notlee
Copy link
Contributor

notlee commented Jan 27, 2025

🕵️ Painful manual Percy / Chromatic mode activated

Weight

On most teasers the weight of the header seems to have increased substantially. Is there a more appropriate usecase? I'd expect `headline` rather than `display`.

Screenshot 2025-01-27 at 17 47 17
Screenshot 2025-01-27 at 17 47 25
Screenshot 2025-01-27 at 17 47 27
Screenshot 2025-01-27 at 17 48 14
Screenshot 2025-01-27 at 17 48 45
Screenshot 2025-01-27 at 17 48 48
Screenshot 2025-01-27 at 17 48 49
Screenshot 2025-01-27 at 17 48 50
Screenshot 2025-01-27 at 17 48 50 1
Screenshot 2025-01-27 at 17 48 51
Screenshot 2025-01-27 at 17 49 01
Screenshot 2025-01-27 at 17 49 03
Screenshot 2025-01-27 at 17 49 05
Screenshot 2025-01-27 at 17 49 06

Colour

In addition to weight there's often colour issues. The main ones to look our for are:
  • Black headings
  • Black time stamps
  • Invisible text (hero, inverse opinion)

Screenshot 2025-01-27 at 17 47 33
Screenshot 2025-01-27 at 17 47 41
Screenshot 2025-01-27 at 17 47 44
Screenshot 2025-01-27 at 17 47 50
Screenshot 2025-01-27 at 17 47 52
Screenshot 2025-01-27 at 17 47 53
Screenshot 2025-01-27 at 17 47 55
Screenshot 2025-01-27 at 17 47 56
Screenshot 2025-01-27 at 17 47 58
Screenshot 2025-01-27 at 17 48 03
Screenshot 2025-01-27 at 17 48 04
Screenshot 2025-01-27 at 17 48 07
Screenshot 2025-01-27 at 17 48 08
Screenshot 2025-01-27 at 17 48 10
Screenshot 2025-01-27 at 17 48 11
Screenshot 2025-01-27 at 17 48 12
Screenshot 2025-01-27 at 17 48 22
Screenshot 2025-01-27 at 17 48 24
Screenshot 2025-01-27 at 17 48 25
Screenshot 2025-01-27 at 17 48 27
Screenshot 2025-01-27 at 17 48 29
Screenshot 2025-01-27 at 17 48 34
Screenshot 2025-01-27 at 17 48 36
Screenshot 2025-01-27 at 17 48 36 1
Screenshot 2025-01-27 at 17 48 37
Screenshot 2025-01-27 at 17 48 38
Screenshot 2025-01-27 at 17 48 39
Screenshot 2025-01-27 at 17 48 40
Screenshot 2025-01-27 at 17 48 43
Screenshot 2025-01-27 at 17 48 44
Screenshot 2025-01-27 at 17 48 46
Screenshot 2025-01-27 at 17 48 47

Size

The size of the heading on some including this one seem quite a bit bigger, do we have a smaller use-case we could use?

Screenshot 2025-01-27 at 17 48 28

Font

The fonts and weight are off on these examples:

  • top-story-hero uses Metric instead of Financier
  • opinion uses Georgia instead of Financier (think I may have spotted some others)

Screenshot 2025-01-27 at 17 47 31
Screenshot 2025-01-27 at 17 48 33

Partner content

Paid post / partner content: The headline and o-teaser__promoted-prefix are much bigger with weight changes, and the font is georgia and not financier for the headline. I'd recommend setting o-teaser__promoted-prefix to the label usecase (smaller, capitalised).
paid-post
promoted-content

@frshwtr
Copy link
Contributor Author

frshwtr commented Jan 28, 2025

Additional colour issue: hero image has mixing issues in small viewports
image

@notlee notlee temporarily deployed to origami-webs-2025-relea-pcml1b January 29, 2025 10:57 Inactive
@notlee notlee temporarily deployed to origami-webs-2025-relea-wowfvv January 29, 2025 11:38 Inactive
@notlee notlee temporarily deployed to origami-webs-2025-relea-wowfvv January 29, 2025 13:36 Inactive
@frshwtr frshwtr force-pushed the 2025-release-use-typography-tokens-in-topper branch from a1370b0 to 1868eee Compare January 29, 2025 13:40
@notlee notlee temporarily deployed to origami-webs-2025-relea-wowfvv January 29, 2025 13:40 Inactive
@notlee notlee temporarily deployed to origami-webs-2025-relea-wowfvv January 29, 2025 13:52 Inactive
@notlee notlee temporarily deployed to origami-webs-2025-relea-wowfvv January 29, 2025 14:05 Inactive
@notlee notlee temporarily deployed to origami-webs-2025-relea-wowfvv January 29, 2025 14:53 Inactive
@notlee notlee self-requested a review January 30, 2025 17:54
Copy link
Contributor

@notlee notlee left a comment

Choose a reason for hiding this comment

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

Great work! 👏 👏 👏 Almost ready to go:

  1. Linting error (fyi I merged 2025-release so all tests should be passing from now on)
  2. Please confirm there's a high priority design ticket in the next sprint to update the use-case / docs across Figma libraries
  3. Underline on this one?
    Screenshot 2025-01-30 at 17 48 13

@notlee notlee temporarily deployed to origami-webs-2025-relea-wowfvv January 30, 2025 19:50 Inactive
@notlee notlee temporarily deployed to origami-webs-2025-relea-wowfvv January 31, 2025 16:04 Inactive
@frshwtr frshwtr merged commit 428eb8d into 2025-release Jan 31, 2025
9 of 11 checks passed
@frshwtr frshwtr deleted the 2025-release-use-typography-tokens-in-topper branch January 31, 2025 16:41
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.

2 participants