-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Add typography heading and body font size scale setting #688
Conversation
…raphy-size-setting-3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an exciting update! Added a few comments :)
@@ -354,73 +354,73 @@ h5, | |||
font-family: var(--font-heading-family); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we have the font size range to 150%
the text gets cut off in the collage
https://screenshot.click/28-03-89dbl-7ohm8.png
This happens when the scales are different: https://screenshot.click/28-13-47dfl-m832n.png
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would prefer to address this in a separate PR. The reason being that this would require some refactoring and re-testing of the collage section. To be clear, this issue already exists with collage since longer product/collection titles could always cause overflow, but you are correct that this issue is made worse by the option to increase font size scale
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue created - #699
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the issue already exists, we can do that. Thanks for creating the issue :)
} | ||
} | ||
|
||
h3, | ||
.h3 { | ||
font-size: 1.7rem; | ||
font-size: calc(var(--font-heading-scale) * 1.7rem); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if it's related to this PR, but is it expected that we have a lot of space between the text and icon? I dont seem to see this with the other items: https://screenshot.click/28-20-7e0f0-z06r0.png
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related to this PR. For <select>
, AFAIK, there's no way to ensure its width fits the currently selected option
h3.caption-large { | ||
font-size: calc(var(--font-heading-scale) * 1.3rem); | ||
} | ||
|
||
.color-foreground { | ||
color: rgb(var(--color-foreground)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The price p
in the gift card doesnt seem to be updating either: https://screenshot.click/28-22-zd38z-zdtqd.png
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be considered "body text" and should be scaling with body_scale
setting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍 but I am seeing this on the gift card page on your PR 🤔 https://screenshot.click/01-51-1spoi-a6twd.png
@@ -49,6 +49,9 @@ | |||
--font-heading-style: {{ settings.type_header_font.style }}; | |||
--font-heading-weight: {{ settings.type_header_font.weight }}; | |||
|
|||
--font-body-scale: {{ settings.body_scale | divided_by: 100.0 }}; | |||
--font-heading-scale: {{ settings.heading_scale | times: 1.0 | divided_by: settings.body_scale }}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am curious as why you are keeping the decimal instead of using divided_by: 100
and times: 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dividing and multiplying integers using Liquid filter would produce an integer result, but for scale, we need decimal values
@@ -71,13 +71,13 @@ | |||
{% if value.count == 0 and value.active == false %}disabled{% endif %} | |||
> | |||
|
|||
<svg width="16" height="16" viewBox="0 0 16 16" aria-hidden="true" focusable="false"> | |||
<svg width="1.6rem" height="1.6rem" viewBox="0 0 16 16" aria-hidden="true" focusable="false"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if we'll need to do this to all of the other svgs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most icons which have width/height defined in CSS already use rem
units and automatically scale with body_scale
setting. Some exceptions (seen in screenshot above) have definitely been missed on my end:
- Share icon doesn't have
rem
width/height defined in CSS - I will fix - Collapsible tab heading icons should scale with
heading_scale
- I will fix also
If you notice any other icons not scaling with the text they accompany, please me know. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated 2ff7cac:
- Share icon now scales with
body_scale
setting - Collapsible tab heading icons now scale with
heading_scale
setting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
Just tested the new changes and all the comments I had were addressed :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you a lot Kai! This is massive! Very excited about this upcoming update and what it will open in flexibility for Dawn!
Did a first pass - I need to do another one, here are my first observations:
- Cart notification product information should use
body_scale
to match the cart- Related to that, something I am wondering is if we should always treat the product title as a heading. I see some different approach on various template, I'd like to have your opinion and perhaps have a wider UX conversation about it.
- Not sure if I saw this covered on another comment but the down-facing caret aren't scaling with the right scaling value for the Collection filters and the nested navigation.
- I don't think this is related but noting in case, filter options aren't centered vertically
- I noticed a
<p>
tag under the Pickup availability drawer over here - Inside the search combo box, the table heading scales with the
heading_scale
value, we are not scaling it by that value in Customer account table and Cart.- This made me realize that styles in Cart, Customer account and Search combo box for the table title aren't the same, I would like to understand the difference when pairing.
I've addressed the feedback. We can discuss further to address some global typography inconsistencies if necessary. Here are some more details on what's been fixed:
Cart notification:
This is now fixed. The caret should now only scale with
This was more related to how the option name was entered in the admin, but is now fixed
I've updated the table heading in predictive search to match the ones on cart page (scales with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Retested everything and it looks good 🚢
thanks for the changes 👍 . Just noticed an internal liquid error on the gift card page (left a comment)
Body text
Before/After |
Awesome Kai, super update! Thanks! Coming later to put my final observations for margins like we discussed IRL. Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you Kai! 🎉
Scaling looks amazing among the changes for line height for body and what is upcoming for headings. I believe checking for headings later once you apply the same logic won't affect my review much.
My review focuses on areas some sections margins don't benefit the scaling flexibility + checking the full set of sections in action.
- Rich text section: The margins becomes too wide on mobile when scaling the body font. Leaving the reading area too narrow and unpleasant to read for customers. Would adjusting the calculation fix the outcome? (
width: calc(100% - 7rem);
instead ofwidth: calc(100% - 8rem);
)- Other areas I noticed a similar need for adjustments are:
- Footer, text blocks gets wrapped too quickly because of that
- Email sign up section, input field and text could take more space horizontally
- Image with text section: Padding could be smaller when the body is scaled
- Collage section: I would tend to suggest to scale this section blocks default headings with the
body_scale
instead of the headings one to not create false expectations for merchants that their text wold be so large. Rationale is to improve the outcome on smaller screens, there is little to no place for content because of the inner padding/margins applied to the different blocks. But that might also need a little UI/UX revision to improve theCollage mobile layout
setting.- Default headings for Collection list too would be inconsistent to express the official outcome once merchants select a real collection. The harmony between the section title and the block's text size once a collection is linked is much better. We can discuss of what would be a better alternative.
- Product cards: When scaling body font, there is a perceptible white space added around the card asset
- Cart template: The table scale is not successful right now. I would suggest if possible not scaling the product image to leave more space for the content. The trash can icon also wraps on a second line, perhaps not scaling the image would keep safe space still.
- Customer account template: Taking a note but will open another issue about it as I believe this is tied to a global change what we will need to update. Table heading from the cart compared to customer account is different in size and inconsistent in letter-spacing. Although we might not want to have those labels too small, they could get some love to improve the template hierarchy.
- Featured collection: Section default content needs more love, vendor name is too close to the card right now compared to the real outcome once you link a collection. (Will open a new issue)
- Featured product:
- Default view, the Sold out label is not vertically aligned with the price (Will open a new issue)
- When the link label is wrapping on two lines or more, the arrow doesn't scale anymore
- Share block: the input goes offscreen when scaling body
@melissaperreault I've addressed the feedback and updated the PR. The following has not been addressed, here are some details:
Agreed, I think the collage section in general would benefit from a slight refactor, especially to accommodate larger text. We can track all fixes in this new issue #699
Aligned on opening an issue to tackle headings globally in the theme for more consistency |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍 Left a few small comments and then I can do a final pass
@@ -22,7 +22,7 @@ h6:empty { | |||
} | |||
|
|||
html { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I adjust the font size in the gift card page I get a Liquid internal error:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created an issue for this #740
Changing any theme setting will actually cause the Liquid error
config/settings_schema.json
Outdated
"theme_documentation_url": "https://help.shopify.com/manual/online-store/themes/os20/themes-by-shopify/dawn", | ||
"theme_support_url": "https://support.shopify.com/" | ||
"theme_documentation_url": "https:\/\/help.shopify.com\/manual\/online-store\/themes\/os20\/themes-by-shopify\/dawn", | ||
"theme_support_url": "https:\/\/support.shopify.com\/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Reverted
<div class="card__inner"> | ||
<div class="card__content"><h2 class="card__text h3">{{ 'onboarding.product_title' | t }}</h2></div> | ||
</div> | ||
</div> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nitpick but on the product page, when scaling the body size, the spacing between some elements feel a bit tight: https://screenshot.click/05-17-vobf4-raq5b.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KaichenWang Is it possible that in this particular block space that the spacing is not scaled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sofiamatulis adjusted spacing for product badges 75a6004. Currently testing a few different pricing scenarios to make sure there's no unwanted impact
@melissaperreault I added some margin around the badges to increase the spacing slightly. But keep in mind that there was only a 5px
margin between the price and the badge. 5px * 1.3
is still only 6.5px
, so smaller margins can appear to not have scaled by much at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! The only thing is that the badge seems to have different alignment on desktop vs mobile: https://screenshot.click/06-30-ceaww-xf5l2.mp4
@melissaperreault not sure if it's intended, if so we can create a separate issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if it's intended, if so we can create a separate issue
Not intended, we could open another issue if we want to wrap this PR sooner.
@@ -58,8 +58,8 @@ | |||
|
|||
.cart-notification__heading .icon-checkmark { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related to this PR but I am seeing this and not on main
🤔
Are you seeing this as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created an issue for this #742
Yes I can reproduce it, but also on main
(pretty much for any non-live instance of the theme)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! 👍
@@ -156,7 +156,7 @@ cart-remove-button .icon-remove { | |||
} | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the filter, when the body is set to the highest size it's cutting the text in some languages (screenshot in French):
https://screenshot.click/05-22-khmva-ekybx.png
Also the focus is cutting of the text next to it: https://screenshot.click/05-23-0npqv-8vhyv.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjusted the spacing and focus ring offset 5e239ef
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great 🎉 thanks for all the changes and the issues created
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -32,9 +36,10 @@ | |||
|
|||
.accordion .icon-accordion { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor bug and a bit of an edge case
Now that accordion icons are based on the size of font heading scale, we'd need to limit the width of .accordion__title
based on that, or it would look like that with long titles:
Solution:
.accordion__title {
display: inline-block;
max-width: calc(100% - 6rem);
could be replaced with:
.accordion__title {
display: inline-block;
max-width: calc(100% - (var(--font-heading-scale) * 4rem));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue created - #750
@@ -447,7 +456,7 @@ blockquote { | |||
.caption { | |||
font-size: 1rem; | |||
letter-spacing: 0.07rem; | |||
line-height: 1.7; | |||
line-height: calc(1 + 0.7 / var(--font-body-scale)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note, not related to this PR: I find this element's font size literally impossible to read on mobile. And my phone has a huuuge almost table-sized screen, so I can't even imagine how hard it is on smaller devices (1rem
= 10px
)
Is this intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to take another look at global styles to make sure the 100% still meets legibility and accessibility norms. Caption is mostly used to support content, but could benefit for being slightly increased. Would be great to have a screenshot and post on that new issue #741 if you have the time 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you all for the detailed reviews and care team! I've also checked around many hidden corners but I suspect this kind of update will uncover edge cases in time that we can fix on a future PR. 🚢 🎉
* Add typography size setting * Adjust setting values * Update font-sizes * Update from Shopify for theme dawn/add-typography-size-setting * Add separate size setting for headings * Use range setting instead of select for typography size * Enable heading font-size scaling * Adjust elements to support scaling * Update arrow icon scaling * Fix typo * Update variable names. Add translations strings for setting labels * Update 16 translation files * Update 3 translation files * Address PR comments. Fix icon size for share and accordion * Address design feedback * Update locales/fi.schema.json * Set cart item font to h4 style * Reduce body line-height as font size scale increases * Adjust global line-height to scale with font-size setting * Address design feedback. Ensure some UI elements do not scale with body font * Revert URL change in settings * Adjust focus and margin of filter select * Adjust product price badge spacing Co-authored-by: shopify-online-store[bot] <79544226+shopify-online-store[bot]@users.noreply.github.com> Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com> Co-authored-by: translation-platform[bot] <35075727+translation-platform@users.noreply.github.com>
Why are these changes introduced?
Fixes #634
What approach did you take?
1.
body_scale
- Add theme typography setting for scaling body text size<html>
base font-sizerem
units, all elements will be "scaled up" when this multiplier is applied - including all body text, icons, padding, margin, height, width, etc.body_scale
value (%)body_scale
/ 100%)0.1rem
equivalent100
1.00
1px
130
1.30
1.3px
2.
heading_scale
- Add theme typography setting for scaling heading text sizebody_scale
affects all CSS elements including headings, this must be negated for headings (divide bybody_scale
)heading_scale
/body_scale
heading_scale
heading_scale
value (%)heading_scale
/body_scale
) ifbody_scale = 100%
0.1rem
equivalent ifbody_scale = 100%
100
1.00
1px
150
1.50
1.5px
3. Adjust site header (and password page header) grid to accommodate larger font-sizes
Other considerations
n/a
Demo links
Checklist