-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Try more universal rich text controls #2085
Changes from all commits
e9cd340
c4479c3
3745d9a
46942e4
16f4d89
ba1699b
de33a83
584803f
558ebec
1105c83
0ee4f5d
9fc69f4
c78bbe3
5006a6a
29691f1
5397daa
96be791
4f41687
4d250af
7824498
d4e822b
a9531cb
a0d1474
3ce78d4
36273a4
2dbbf2e
a4e3421
ef14dcf
74b7468
aa068d3
d891746
c8bd24c
d70dab2
fd39a8e
dd2942b
cd07536
1be9b29
09c1882
5914938
4fd4772
d6e6f33
1cf7b3b
610e6c4
2659eba
4a472d5
2428e48
5e1989a
8652936
d956d6f
4eae366
37e7ca8
4a018e9
c5535db
7d9f077
00292c9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -98,12 +98,10 @@ | |||||
{%- for block in section.blocks -%} | ||||||
{%- case block.type -%} | ||||||
{%- when 'heading' -%} | ||||||
<h2 class="banner__heading {{ block.settings.heading_size }}" {{ block.shopify_attributes }}> | ||||||
<span>{{ block.settings.heading | escape }}</span> | ||||||
</h2> | ||||||
<h2 class="banner__heading inline-richtext {{ block.settings.heading_size }}" {{ block.shopify_attributes }}>{{ block.settings.heading }}</h2> | ||||||
{%- when 'text' -%} | ||||||
<div class="banner__text {{ block.settings.text_style }}" {{ block.shopify_attributes }}> | ||||||
<span>{{ block.settings.text | escape }}</span> | ||||||
<div class="banner__text rte {{ block.settings.text_style }}" {{ block.shopify_attributes }}> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Just wondering if the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch! In this specific case, the text mimics There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the context, Kjell |
||||||
<p>{{ block.settings.text }}</p> | ||||||
</div> | ||||||
{%- when 'buttons' -%} | ||||||
<div | ||||||
|
@@ -338,7 +336,7 @@ | |||||
"limit": 1, | ||||||
"settings": [ | ||||||
{ | ||||||
"type": "text", | ||||||
"type": "inline_richtext", | ||||||
"id": "heading", | ||||||
"default": "Image banner", | ||||||
"label": "t:sections.image-banner.blocks.heading.settings.heading.label" | ||||||
|
@@ -371,7 +369,7 @@ | |||||
"limit": 1, | ||||||
"settings": [ | ||||||
{ | ||||||
"type": "text", | ||||||
"type": "inline_richtext", | ||||||
kjellr marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
"id": "text", | ||||||
"default": "Give customers details about the banner image(s) or content on the template.", | ||||||
"label": "t:sections.image-banner.blocks.text.settings.text.label" | ||||||
|
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.
Nit: Does this need to be in a media query? If it's true all the way from 0 - 749px then it should be outside of a media query.
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.
It doesn't technically need to. But in this case, we have multiple rules that are inherited correctly on desktop by default.
We could definitely remove the
max-width
media query here, but then we'd need to take corrective action on 750 and above to replicate the way it would've normally appeared. This doubled the CSS rules, and seemed heavy-handed to me (especially considering there are lots of other(max-width: 749px)
breakpoints used in this file already).Basically, it's a choice between this:
... or this:
inherit
doesn't work in this case, so you're stuck specifying the same variables that are assigned in the original rules fromcomponent-rte.css
andbase.css
. All in all, I felt that just using themax-width
rule was simpler. Happy to change if you feel strongly though.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.
Thanks for looking into this. In general I think we should pick either
min-width
ormax-width
and stick to it but we already do this all over the CSS. I won't be a stickler here.