Details

Reviewers

• jacek
tomek
varun
• benschac

Commits

rCOMM5a78c17cf968: [landing] Keep hero `TextLoop` to two lines

Summary

Use clamp to interpolate between font-sizes that ensure that things remain on two lines

now depends on D3430

Test Plan

Set the text to "gaming guilds" (the longest one) and made sure the text never broke to three lines. Also checked iPhone 13 mini to make sure that it looks okay at a very low breakpoint

Diff Detail

Repository

rCOMM Comm

Branch

master

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

atul created this revision.Mar 9 2022, 3:38 PM

Herald added subscribers: • karol, • adrian, ashoat. · View Herald TranscriptMar 9 2022, 3:38 PM

Harbormaster completed remote builds in B7272: Diff 10254.Mar 9 2022, 3:43 PM

atul requested review of this revision.Mar 9 2022, 3:43 PM

atul edited the test plan for this revision. (Show Details)Mar 9 2022, 3:45 PM

use clamp instead

atul edited the summary of this revision. (Show Details)Mar 9 2022, 6:12 PM

atul edited the test plan for this revision. (Show Details)

atul added inline comments.

landing/hero-content.css
32 ↗	(On Diff #10259)	I'm almost positive this was unintentional and some Figma artifact that snuck in

Harbormaster failed remote builds in B7277: Diff 10259!Mar 9 2022, 6:15 PM

update values

symmetrical

Harbormaster completed remote builds in B7278: Diff 10260.Mar 9 2022, 7:14 PM

Harbormaster completed remote builds in B7279: Diff 10261.Mar 9 2022, 7:17 PM

atul edited the summary of this revision. (Show Details)Mar 9 2022, 8:16 PM

atul edited the test plan for this revision. (Show Details)

atul retitled this revision from [landing] Keep hero `TextLoop` to two lines for desktop (>1099px) breakpoints to [landing] Keep hero `TextLoop` to two lines.Mar 9 2022, 9:09 PM

There's a lot going on in this diff and in such a case we should explain it to the reviewers. In this case, there's a relationship between these two formulas and font-size for different widths. Also, the formulas look quite arbitrary - it should either be stated, or it should be explained that there's a deeper reason behind choosing these exact values.

landing/hero-content.css
5–9 ↗	(On Diff #10261)	Are these experimentally chosen or is there a relationship between them? We should choose these so that the transition between 1100px and 1099px is smooth. To have that we need to ensure that `0.0715rem + 5.715vw = 0.0715rem + 0.05715 * 1099px` is equal to `-0.11rem + 3.66vw = -0.11rem + 0.0366 * 1100px`. So to correctly determine these values we need to know what is `font-size` for different max-width. These formulas tightly depend on exact values of `font-size`, so we should do something that ensures that they remain in sync. One possible option might be to use `px` but it has a lot of disadvantages. We can also add comments in both places, to warn a developer that updating in one place will require updating the other.

This revision now requires changes to proceed.Mar 10 2022, 2:07 AM

• benschac requested changes to this revision.Mar 10 2022, 10:08 AM

• benschac added inline comments.

landing/hero-content.css
5–9 ↗	(On Diff #10261)	Following up with Tomik's comment. How did you get these values? I do think using clamp is the right solution. https://modern-fluid-typography.vercel.app -- maybe looking into something like this would be helpful.

Sorry yeah, for @media (max-width: 1099px) I interpolated over values that I found kept things to two lines. I used https://css-tricks.com/linearly-scale-font-size-with-css-clamp-based-on-the-viewport/ to figure out what values to use.

24px font-size at <=399px width
64px font-size at >=1099px width

Then converted those units to rem assuming 1rem = 16px

1.5rem font-size at <=25rem width
4rem font-size at >= 68.75rem width

and then plugged it into the math they provided:

and it worked as expected..

As for the font-size outside of the media-query, I couldn't get things to work for the life of me... so I just experimented with values until I got the desired result. I think I misunderstood rem and relative units generally, I'll take another look today... the tool @benschac linked looks promising

We should choose these so that the transition between 1100px and 1099px is smooth.

We actually don't have to worry about this because the layout reflows when we make the "transition" between those breakpoints

The issue here is with the following in global.css:html:

/* Fallback styling if clamp isn't supported  */
  font-size: 1rem;
  /* https://css-tricks.com/simplified-fluid-typography/ */
  font-size: clamp(
    var(--min-font-size),
    calc(0.75rem + 1vw),
    var(--max-font-size)
  );

which changes the definition of 1rem = 16px to something that varies with the width. We shouldn't be doing this, as it messes up the math downstream.

I've created an issue to address this: https://linear.app/comm/issue/ENG-867/remove-font-size-clamp-from-globalcss

We can either let this diff through with values that we've found work experimentally, or we can make that change and do it "correctly." The issue is that after removing that global clamp, we'll need to change the other 4-5 occurrences of font-size: clamp and make sure they continue to look as expected.

cc: @ashoat @palys-swm @benschac

rebase

Harbormaster completed remote builds in B7347: Diff 10348.Mar 14 2022, 10:18 AM

In D3392#92367, @atul wrote:
The issue here is with the following in global.css:html:
/* Fallback styling if clamp isn't supported  */
  font-size: 1rem;
  /* https://css-tricks.com/simplified-fluid-typography/ */
  font-size: clamp(
    var(--min-font-size),
    calc(0.75rem + 1vw),
    var(--max-font-size)
  );
which changes the definition of 1rem = 16px to something that varies with the width. We shouldn't be doing this, as it messes up the math downstream.

I've created an issue to address this: https://linear.app/comm/issue/ENG-867/remove-font-size-clamp-from-globalcss

We can either let this diff through with values that we've found work experimentally, or we can make that change and do it "correctly." The issue is that after removing that global clamp, we'll need to change the other 4-5 occurrences of font-size: clamp and make sure they continue to look as expected.

cc: @ashoat @palys-swm @benschac

I'm okay with the experimental fix for the moment. I'd rather go back and really fix the typography in the near future. Sorry for introducing this concept the wrong way!

• benschac accepted this revision.Mar 14 2022, 2:27 PM

Discussed this with @atul today, I agree that making 1rem dynamic makes it harder to denominate things in terms of rems, especially when those second-order things need to be dynamically sized

In D3392#91877, @atul wrote:

Sorry yeah, for @media (max-width: 1099px) I interpolated over values that I found kept things to two lines. I used https://css-tricks.com/linearly-scale-font-size-with-css-clamp-based-on-the-viewport/ to figure out what values to use.

24px font-size at <=399px width
64px font-size at >=1099px width

Then converted those units to rem assuming 1rem = 16px

1.5rem font-size at <=25rem width
4rem font-size at >= 68.75rem width

and then plugged it into the math they provided:

and it worked as expected..

As for the font-size outside of the media-query, I couldn't get things to work for the life of me... so I just experimented with values until I got the desired result. I think I misunderstood rem and relative units generally, I'll take another look today... the tool @benschac linked looks promising

We should choose these so that the transition between 1100px and 1099px is smooth.

We actually don't have to worry about this because the layout reflows when we make the "transition" between those breakpoints