-
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 heading font size setting in Rich text section #188
Conversation
sections/rich-text.liquid
Outdated
@@ -9,7 +9,7 @@ | |||
{%- for block in section.blocks -%} | |||
{%- case block.type -%} | |||
{%- when 'heading' -%} | |||
<h2 class="{% if block.settings.heading_size == 'small' %}h2{% else %}h1{% endif %}" {{ block.shopify_attributes }}>{{ block.settings.heading | escape }}</h2> | |||
<h2 class="{% if block.settings.heading_size == 'large' %}h1{% elsif block.settings.heading_size == 'medium' %}h2{% else %}h3{% endif %}" {{ block.shopify_attributes }}>{{ block.settings.heading | escape }}</h2> |
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 don't know why but the option doesn't output the right heading size, they look too small now. After investigating it seems like the options before were already perfect and using respectively H1 and H2, I am sorry for this confusion! Large should be using the H0. It was planned to offer a taller heading size but I don't recall what happened to this exploration. It would be key for the theme to have this option.
Keep the size like before for Small
and Medium
but please add a new heading style H0 for large heading to apply to the Large
option:
- Desktop is 52px
- Mobile is 40px
Recap:
- Small = H2
- Medium = H1
- Large = H0
cc. @nicklepine @Oliviammarcello @wiktoriaswiecicka, I couldn't find the updated version of the new Typography changes like it was supposed to be done to reference in those circumstances. We'll need this to inform and support our reviews please.
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 made the changes 👍
13db0ff
to
cf9b9a3
Compare
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.
Tested it and it looks good. Nice - I like the approach 👍 🚢
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 Thomas! LGTM! 🎉
* main: Adjust announcement bar arrow icon (Shopify#253) Add heading font size setting in Rich text section (Shopify#188)
* Add heading setting in Rich text * adjust style class * Changed the approach to not require a large set of if/elsif Co-authored-by: tyleralsbury <tyler.alsbury@shopify.com>
Why are these changes introduced?
Fixes #186
The goal of this PR is to add an additional heading setting in the Rich text section.
What approach did you take?
Design notes
Small:
h3
(did not see in the issue which style it was using, so I assume it'sh3
)Medium:
h2
Large:
h3
Demo links
Checklist