Page MenuHomePhabricator

[landing] Keep hero `TextLoop` to two lines
ClosedPublic

Authored by atul on Mar 9 2022, 3:38 PM.
Tags
None
Referenced Files
F3449225: D3392.id10453.diff
Thu, Dec 12, 10:28 AM
F3449170: D3392.diff
Thu, Dec 12, 8:49 AM
Unknown Object (File)
Sat, Dec 7, 6:52 PM
Unknown Object (File)
Sat, Nov 16, 10:20 PM
Unknown Object (File)
Fri, Nov 15, 6:37 PM
Unknown Object (File)
Fri, Nov 15, 6:36 PM
Unknown Object (File)
Fri, Nov 15, 6:36 PM
Unknown Object (File)
Fri, Nov 15, 9:00 AM

Details

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 requested review of this revision.Mar 9 2022, 3:43 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

atul edited the summary of this revision. (Show Details)

update values

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
tomek requested changes to this revision.Mar 10 2022, 2:07 AM

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 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:

0bb6.png (830×2 px, 124 KB)

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

atul requested review of this revision.EditedMar 14 2022, 8:17 AM

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

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!

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:

0bb6.png (830×2 px, 124 KB)

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

Thanks for the explanation!

This revision is now accepted and ready to land.Mar 15 2022, 10:51 AM
This revision was landed with ongoing or failed builds.Mar 16 2022, 12:14 PM
This revision was automatically updated to reflect the committed changes.