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

Club: implementing fluid typography #6262

Merged
merged 12 commits into from
Aug 5, 2022
Merged

Conversation

matiasbenedetto
Copy link
Member

Changes proposed in this Pull Request:

Club theme: Implement the fluid type sizes API in theme.json.

ℹ️ Notes about how this new feature works in practice:

  • fluid.min and fluid.max, obviously, works as max and min font sizes. min is applied in all the viewports below 768px wide. This value is enlarged progressively when the viewport grows. If the viewport is more that 1600px wide max is applied as font size.
  • I've tested the API without Gutenberg activated and the size key is used as a fallback value. So leveraging the API won't imply a broken site for users not using the Gutenberg plugin.

This PR is an alternative approach to this one #6255.
Thanks, @madhusudhand for working on it!

⚠️ We still need to add the line height settings so I add this as a draft to discuss the fluid typography implementation.

Related issue(s):

#6213

club/theme.json Outdated
"min": "1.25rem",
"max": "2.5rem"
},
"slug": "default-title",
Copy link
Member Author

Choose a reason for hiding this comment

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

I've added a font size not explicitly defined in the mockups but still widely used there. I refer to the titles and other elements that are 20px on mobile and 40px on desktop. I implemented that as default-title.

@madhusudhand
Copy link
Contributor

Thanks for bring it in. It eliminates custom calculations and works great!
Only problem I see is that the size dropdown now has this huge list, we keep all sizes under preset font sizes.

image

Does custom config or block typography support fluid?

@matiasbenedetto
Copy link
Member Author

thanks for the feedback @madhusudhand!

Only problem I see is that the size dropdown now has this huge list, we keep all sizes under preset font sizes

Yep, the list is a little bit long but I think It won't hurt the user experience.

Does custom config or block typography support fluid?

Nope, this feature is not working yet on custom or blocks typography definition. So our only way to leverage it is using it inside fontSizes definition.

@matiasbenedetto matiasbenedetto added this to the club milestone Jul 21, 2022
Copy link
Contributor

@madhusudhand madhusudhand left a comment

Choose a reason for hiding this comment

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

Also consider adding lineHeight and letterSpacing for various blocks.

club/style.css Outdated Show resolved Hide resolved
club/theme.json Outdated Show resolved Hide resolved
@matiasbenedetto
Copy link
Member Author

matiasbenedetto commented Jul 22, 2022

Also consider adding lineHeight and letterSpacing for various blocks.

I added the letter spacing settings, I prefer to work the line height on a separate PR when we have more templates finished and we can test it better. I created an issue in the Club milestone about that: #6274

@scruffian
Copy link
Member

I think it would be interesting to try this without any min/max settings - that way we'll see how well the defaults work and if we want to tweak them...

Copy link
Contributor

@madhusudhand madhusudhand left a comment

Choose a reason for hiding this comment

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

Following is the overall feedback:

Paragraphs:

✅ Small
❌ Default -> current: 15-30px expected: 16-20px
❌ Medium -> current: 18-36px expected: 20-24px
❌ Large -> current: 21-42px expected: 28px
❌ Extra large -> current: 30-60px expected: 40px

Headings:

✅ H1
✅ H2
❌ H3 -> current: 20-40px expected: 30-40px
✅ H4
❌ H5 -> current: 15-30px expected: 20px
✅ H6

also font-weight is not matching for the headings

❌ post title: commented inline
❌ site-title and nav menu sizes are smaller

club/theme.json Show resolved Hide resolved
@matiasbenedetto
Copy link
Member Author

Following is the overall feedback:

Paragraphs:

white_check_mark Small x Default -> current: 15-30px expected: 16-20px x Medium -> current: 18-36px expected: 20-24px x Large -> current: 21-42px expected: 28px x Extra large -> current: 30-60px expected: 40px

Headings:

white_check_mark H1 white_check_mark H2 x H3 -> current: 20-40px expected: 30-40px white_check_mark H4 x H5 -> current: 15-30px expected: 20px white_check_mark H6

also font-weight is not matching for the headings

x post title: commented inline x site-title and nav menu sizes are smaller

Thanks for testing @madhusudhand, the problem with those sizes is caused by the implementation of Gutenberg Fluid typography API that doesn't seem to be working as expected. I reported it here: WordPress/gutenberg#42698

@madhusudhand
Copy link
Contributor

Let's merge #6255 to unblock the development and park this PR until issues gets fixed in gutenberg?

@matiasbenedetto
Copy link
Member Author

Let's merge #6255 to unblock the development and park this PR until issues gets fixed in gutenberg?

I just implemented the new flag to avoid fluid sizes and recently merged to Gutenberg.

@matiasbenedetto
Copy link
Member Author

The implementation seems to be working fine:

Paragraphs:

small: 14px - 16px -> 0.875rem - 1rem
default: 14px - 20px -> 0.875rem - 1.25rem
medium: 20px - 24px -> 1.25rem - 1.5rem
large: 28px -> 1.75 rem
x-large: 40px -> 2.5rem

Headers:

h1: 60px - 130px -> 3.75rem - 8.125rem
h2: 40px - 110px -> 2.5rem - 6.875rem
h3: 30px - 40px -> 1.875rem - 2.5rem
h4: 28px - 32px -> 1.75rem - 2rem
h5: 20px - 20px -> 1.25rem - 1.25rem
h6: 14px - 16px -> 0.875rem - 1rem (same as small)

Default Title: 20px - 40px -> 1.25rem - 2.5rem

Tested with this markup and working as expected.

<!-- wp:paragraph {"fontSize":"small"} -->
<p class="has-small-font-size">Small paragraph</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph -->
<p>Default Paragraph</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph {"fontSize":"medium"} -->
<p class="has-medium-font-size">Medium Paragraph</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph {"fontSize":"large"} -->
<p class="has-large-font-size">Large Paragraph</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph {"fontSize":"x-large"} -->
<p class="has-x-large-font-size">X-large paragraph</p>
<!-- /wp:paragraph -->

<!-- wp:heading {"level":1,"fontSize":"header-one"} -->
<h1 class="has-header-one-font-size">Heading One</h1>
<!-- /wp:heading -->

<!-- wp:heading -->
<h2>Header Two</h2>
<!-- /wp:heading -->

<!-- wp:heading {"level":3,"fontSize":"header-three"} -->
<h3 class="has-header-three-font-size">Heading Three</h3>
<!-- /wp:heading -->

<!-- wp:heading {"level":4} -->
<h4>Heading Four</h4>
<!-- /wp:heading -->

<!-- wp:heading {"level":5} -->
<h5>Heading Five</h5>
<!-- /wp:heading -->

<!-- wp:heading {"level":6} -->
<h6>Heading Six</h6>
<!-- /wp:heading -->

<!-- wp:paragraph {"fontSize":"default-title"} -->
<p class="has-default-title-font-size">Default title</p>
<!-- /wp:paragraph -->

club/theme.json Outdated
"min": "3.75rem",
"max": "8.125rem"
},
"slug": "header-one",
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure its a good idea to implement these non-standard typography options. This will expose these to users in the block editor and they will be able to select them for their content. Then when they switch themes they will lose these settings and the fonts will revert to a default size. I'd recommend either moving these to custom, or simply defining them directly on the patterns and templates as needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Created an enhancement request here WordPress/gutenberg#42694
There is an idea of being able to define hidden sizes. It might be a solution around this.

Copy link
Member

Choose a reason for hiding this comment

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

I'm also not sure it's a good idea to include the non-standard options. Should we move the non-standard options to where they're being used (e.g. to the heading elements, directly in the templates, etc) for now, until we have a solution like the hidden sizes? Then we can progress this PR.

@pbking
Copy link
Contributor

pbking commented Aug 1, 2022

Moving this one back to in-progress to resolve the conflicts and implement the non-standard sizes directly on the template.

@matiasbenedetto matiasbenedetto force-pushed the club/fluid-typography branch 2 times, most recently from 10a463e to e650612 Compare August 4, 2022 14:34
@matiasbenedetto
Copy link
Member Author

In settings.typography.fontSizes I let just the 4 sizes defined in the Gutenberg default theme.json file. I moved the other dynamic definitions to custom or to the headings (h1,h2,etc) definitions using clamp because the automatic dynamic calculation is not available outside the main fontSizes array.

If WordPress/gutenberg#42694 or any other effort around supporting dynamic font sizes outside the settings.typography.fontSizes array is merged we can change that.

Copy link
Contributor

@madhusudhand madhusudhand left a comment

Choose a reason for hiding this comment

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

Resolved the conflicts and verified the changes.
LGTM!

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.

None yet

5 participants