Try more universal rich text controls #2085

kjellr · 2022-11-01T19:14:48Z

PR Summary:

This PR swaps text input settings out for inline_richtext controls in the following places:

~~Collapsible content > Heading~~
~~Collapsible content > Row name~~
Collection List > Heading
Contact Form > Heading
Featured Blog > Heading
Featured Collection > Heading
Featured Product > Text Block > Heading
Featured Product > Icon with Text > Headings 1-3
Featured Product > Collapsible Tab > Heading
Footer > Link List Heading
Footer > Text Block > Heading
Footer > Newsletter Heading
Image Banner > Heading
Image Banner > Text
Image with Text > Heading
Main List Collections > Heading
Main Product > Text Block > Heading
Main Product > Icon with Text > Headings 1-3
~~Main Product > Collapsible Tab > Heading~~
Multicolumn > Heading
Multicolumn > Column > Title
Multimedia Collage > Heading
Newsletter > Heading
Product Recommendations > Heading
Slideshow > Heading
Slideshow > Subheading
Video > Heading

Screenshot

🎥 Demo video

09-54-vty50-ilg03.mp4

Why are these changes introduced?

We currently don't universally allow merchants to perform simple inline text formatting (bold, italic, etc.). This is only turned on for a seemingly-random subset of different Sections and Blocks. This PR is an attempt to make those controls more universal.

Fixes #2086 (and also attempts to fix that problem for other similar situations).

What approach did you take?

In general, I manually added various sections to my store and swapped out text inputs for richtext or inline_richtext whenever there wasn't a clear reason to avoid doing so.

Visual impact on existing themes

Based on my testing, I'm pretty sure this should have no impact — it only adds functionality.

Demo links

Demo Store

Decision Log

#	Decision	Alternatives	Rationale	Downsides
1	Do not allow `inline_richtext` in collapsible row headings.	Add `inline_richtext` in collapsible row headings.	We decided not to enable this because it interferes with the clickable expanding/collapsing hover states.	Less flexibility in customizing the text within collapsible row headings.
2	Do not allow `inline_richtext` in collapsible row headings within `main-product.liquid`.	Allow it there.	Although collapsible rows here do not have underlined hover states like they do in the Collapsible Row block, we decided that for consistency with decision No. 1, we should leave `inline_richtext` functionality out of here too.	Less flexibility in customizing the text within collapsible row headings.
3	Do not update the existing `richtext` field for Rich Text Section's Heading block to use `inline_richtext` instead.	Update it	Updating this field would introduce the potential of losing merchant-entered data on theme upgrade. We would like to explore possible solutions to this this in a followup PR.	Unlike most other headings fields, merchants can enter multiple paragraphs into this field. These will continue to be stripped out on render.
4	We are going to punt the decision of whether to commit to using `richtext` in the Image Banner and SlideShow Slide description fields.	Enable richtext for these areas.	We'd considered doing this to align on using `richtext` here to align with the Image with Text Section's handling of this area, but ran into bugs that would require more intense work. Upgrading to just `inline_richtext` is straightforward and introduces none of these issues, so we'll take this as a first step.	Merchants cannot use multi-paragraph text in the description fields for Slideshow and Image Banner.

Checklist

Added PR summary for release notes
Requested review from UX (Only for changes that are affecting the experience or perceivable visual details)
Created a ticket for the help.shopify.com documentation team about updates to theme settings. (Internal-only task)
Followed theme code principles
Linted with Theme Check
Tested on mobile
Tested on multiple browsers
Tested for accessibility

This reverts commit e9cd340.

kjellr · 2022-11-02T12:06:51Z

One thing to look out for in reviews:

Since some of these did not allow for inline links before, we need to make sure that they all include appropriate styles for any links that might be added in.

sections/collapsible-content.liquid

elyseviotto · 2022-11-08T18:51:08Z

Just catching up on this topic!
I found interesting context in Slack about RTEs VS text / textarea: see https://shopify.slack.com/archives/CB0JT58RE/p1665156099604899

ludoboludo · 2023-02-15T16:17:28Z

Would you folks mind looking into that so we can close this out? Thank you! 🙌

Yeah I was trying to find folks that could potentially help us out from other team to see what kind of impact on the rendering this has.
My thought is that it shouldn't block this from merging 🤷 Because if anything we should look into more efficient rendering instead of limiting the options we give to merchants.

We'll keep you posted once we can test more and confirm whether or not it's a concerning hit on performance 👍

ludoboludo · 2023-02-17T16:17:30Z

Alrighty, I've explored/tested render times and it didn't to be an issue. I tried with a little over 20 sections that had text headings and tested again with the same setup but with inline_richtext.
Used an internal tool to check the total time to render liquid and numbers were:

text - 445ms
inline_richtext - 365ms

I'll re review the PR either today or next week so we can finally get it into the theme 👍

stufreen · 2023-02-17T16:28:33Z

@ludoboludo @kjellr Yup I agree - after Ludo ran many tests with storefront renderer using a LOT of inline-richtext settings I don't see any performance impacts on rendering time. We can keep an eye on lighthouse scores and real user metrics but I really doubt we'll see any impact, especially since most pages and sections will be cached.

kjellr · 2023-02-17T17:22:02Z

Wonderful, thank you both!

ludoboludo

Looking good, a couple small comments for nitpicks. But otherwise good.

assets/section-image-banner.css

Co-authored-by: Ludo <ludo.segura@shopify.com>

ludoboludo

kjellr · 2023-02-21T16:20:10Z

@ludoboludo any idea why the PR is failing tests all of a sudden? Not much has changed in the PR lately. 🤔

Edit: All set, just needed to be re-run. thanks! 🙌

stufreen

LGTM with comments addressed

🎩 on os2-demo store on Chrome and looks great

stufreen · 2023-02-22T15:40:07Z

assets/section-image-banner.css

+@media screen and (max-width: 749px) {
+  .banner:not(.slideshow) .rte a,
+  .banner:not(.slideshow) .inline-richtext a:hover,
+  .banner:not(.slideshow) .rte a:hover {
+    color: currentColor;
+  }
+}


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.

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:

@media screen and (max-width: 749px) { .banner:not(.slideshow) .rte a, .banner:not(.slideshow) .inline-richtext a:hover, .banner:not(.slideshow) .rte a:hover { color: currentColor; } }

... or this:

.banner:not(.slideshow) .rte a, .banner:not(.slideshow) .inline-richtext a:hover, .banner:not(.slideshow) .rte a:hover { color: currentColor; } @media screen and (min-width: 750px) { .banner:not(.slideshow) .rte a, .banner:not(.slideshow) .rte a:hover { color: rgba(var(--color-link),var(--alpha-link)); } .banner:not(.slideshow) .inline-richtext a:hover { color: var(--color-link); }

inherit doesn't work in this case, so you're stuck specifying the same variables that are assigned in the original rules from component-rte.css and base.css. All in all, I felt that just using the max-width rule was simpler. Happy to change if you feel strongly though.

Thanks for looking into this. In general I think we should pick either min-width or max-width and stick to it but we already do this all over the CSS. I won't be a stickler here.

stufreen · 2023-02-22T15:43:21Z

assets/base.css

  color: rgb(var(--color-link));
  text-decoration-thickness: 0.2rem;
 }

+.inline-richtext a,


I think this is always going to override the rule on line 734. Can you delete the rule above or combine?

That's true! Writing it this way was more concise, but it required an override and was perhaps more difficult to interpret overall. I've tidied this up 00292c9.

stufreen · 2023-02-22T15:58:36Z

sections/image-banner.liquid

-            <h2 class="banner__heading {{ block.settings.heading_size }}" {{ block.shopify_attributes }}>
-              <span>{{ block.settings.heading | escape }}</span>
+            <h2 class="banner__heading inline-richtext {{ block.settings.heading_size }}" {{ block.shopify_attributes }}>
+              <span>{{ block.settings.heading }}</span>


Question: Why do you need the span elements here and below? Let's remove unless necessary.

I don't think we need the span element here, since it doesn't serve any sort of purpose inside of the h2.

For semantic reasons, the text below should be wrapped in something other than a div — the inline richtext control doesn't provide outer tags. I guess a paragraph tag makes the most sense? I've made that change in c5535db and also expanded it to cover the slideshow slide, since that has basically the same markup and purpose. These updates result in no visual changes.

sections/multicolumn.liquid

sections/newsletter.liquid

ludoboludo

One comment but can be tackled as a follow up. I created a PR to change the rich text heading to a inline_richtext type. So it could be done/looked into there 👍

ludoboludo · 2023-02-22T20:19:08Z

sections/slideshow.liquid

@@ -1,4 +1,5 @@
 {{ 'section-image-banner.css' | asset_url | stylesheet_tag }}
+{{ 'component-rte.css' | asset_url | stylesheet_tag }}


The footer is loading the component-rte.css asset. I think we should just remove it from anywhere else since the footer is always present anyway 🤷
Otherwise, not a huge deal but this in the inspect tool is pretty annoying:

Screenshot

kjellr · 2023-02-23T13:26:45Z

Thank you for helping get this out the door! 👏

* shopify/main: update version and release notes (Shopify#2327) add zoom on hover and show spinner when image loading (Shopify#2314) Try more universal rich text controls (Shopify#2085) Hosted video support in video section (Shopify#2248)

* Update image-banner.liquid * Update main-product.liquid * Update collapsible-content.liquid * Update multicolumn.liquid * Update collection-list.liquid * Update featured-collection.liquid * Update main-list-collections.liquid * Update featured-blog.liquid * Update featured-product.liquid * Update footer.liquid * Update newsletter.liquid * Update slideshow.liquid * Revert "Update image-banner.liquid" This reverts commit e9cd340. * Fix image banner implementation. * Update collage.liquid * Update collapsible_tab in Main Product Section * Update image-with-text.liquid * Update product-recommendations.liquid * Update contact-form.liquid * Update video.liquid Update video.liquid * Update collapsible-content.liquid * Add a little bit of inline rte CSS. To ensure links appear correctly. * Update rich-text heading to use inline RTE instead of normal RTE. * Remove paragraphs. * Remove paragraph tags from homepage rich text heading. * Move inline rte code inside of the main rte css file. * Remove unintentional whitespace changes. * Whitespace update. * Code cleanup. * re add classes in icon with text snippet after rebase * Move link styles to base.css * Remove unnecessary asset loading. * Use RTE styles for the slideshow subhead instead. * Slideshow and image banner special cases. * Remove richtext from collapsible row. * Tidying up. * Remove component-rte.css from collage.liquid It doesn't use a richtext component. * Load the rte styles in slideshow.liquid. * Remove unnecessary rule. * Update classname to inline-richtext. * Revert back to the existing richtext field in rich-text.liquid heading. * Use inline-heading class for richtext heading. To retain the same hover color as other headings. * Fix link color inconsistencies. * Move banner--desktop-transparent styles. * Combine CSS Rules. Co-authored-by: Ludo <ludo.segura@shopify.com> * Combine CSS rules. Co-authored-by: Ludo <ludo.segura@shopify.com> * Update wrapper tags for Image Banner & Slideshow. * Remove duplicate calls to component-rte.css * Reformat link color rules to be more readable. --------- Co-authored-by: Ludo <ludo.segura@shopify.com>

kjellr added 14 commits November 1, 2022 13:12

Update image-banner.liquid

e9cd340

Update main-product.liquid

c4479c3

Update collapsible-content.liquid

3745d9a

Update multicolumn.liquid

46942e4

Update collection-list.liquid

16f4d89

Update featured-collection.liquid

ba1699b

Update main-list-collections.liquid

de33a83

Update featured-blog.liquid

584803f

Update featured-product.liquid

558ebec

Update footer.liquid

1105c83

Update newsletter.liquid

0ee4f5d

Update slideshow.liquid

9fc69f4

Revert "Update image-banner.liquid"

c78bbe3

This reverts commit e9cd340.

Fix image banner implementation.

5006a6a

melissaperreault self-requested a review November 1, 2022 19:49

Update collage.liquid

29691f1

kjellr self-assigned this Nov 1, 2022

kjellr added Category: Enhancement New feature or request 🎨 Design By: Shopify and removed By: Shopify labels Nov 1, 2022

kjellr marked this pull request as ready for review November 1, 2022 20:09

kjellr added 4 commits November 2, 2022 07:54

Update collapsible_tab in Main Product Section

5397daa

Update image-with-text.liquid

96be791

Update product-recommendations.liquid

4f41687

Update contact-form.liquid

4d250af

melissaperreault reviewed Nov 8, 2022

View reviewed changes

sections/collapsible-content.liquid Outdated Show resolved Hide resolved

github-actions bot added the cla-needed label Nov 8, 2022

kjellr requested review from andrewetchen, ludoboludo and stufreen February 15, 2023 13:12

ludoboludo previously approved these changes Feb 17, 2023

View reviewed changes

assets/section-image-banner.css Outdated Show resolved Hide resolved

assets/section-image-banner.css Outdated Show resolved Hide resolved

Combine CSS Rules.

37e7ca8

Co-authored-by: Ludo <ludo.segura@shopify.com>

kjellr dismissed ludoboludo’s stale review via 37e7ca8 February 21, 2023 13:19

Combine CSS rules.

4a018e9

Co-authored-by: Ludo <ludo.segura@shopify.com>

kjellr requested a review from ludoboludo February 21, 2023 13:20

ludoboludo previously approved these changes Feb 21, 2023

View reviewed changes

stufreen self-assigned this Feb 21, 2023

stufreen previously approved these changes Feb 22, 2023

View reviewed changes

Update wrapper tags for Image Banner & Slideshow.

c5535db

kjellr dismissed stale reviews from stufreen and ludoboludo via c5535db February 22, 2023 16:40

kjellr added 2 commits February 22, 2023 11:42

Remove duplicate calls to component-rte.css

7d9f077

Reformat link color rules to be more readable.

00292c9

stufreen self-requested a review February 22, 2023 17:54

stufreen approved these changes Feb 22, 2023

View reviewed changes

kjellr requested a review from ludoboludo February 22, 2023 18:28

ludoboludo approved these changes Feb 22, 2023

View reviewed changes

kjellr merged commit 9f2753f into main Feb 23, 2023

kjellr deleted the try-rich-text-in-more-places branch February 23, 2023 13:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try more universal rich text controls #2085

Try more universal rich text controls #2085

kjellr commented Nov 1, 2022 •

edited

Loading

kjellr commented Nov 2, 2022 •

edited

Loading

elyseviotto commented Nov 8, 2022

ludoboludo commented Feb 15, 2023 •

edited

Loading

ludoboludo commented Feb 17, 2023

stufreen commented Feb 17, 2023 •

edited

Loading

kjellr commented Feb 17, 2023

ludoboludo left a comment

ludoboludo left a comment

kjellr commented Feb 21, 2023 •

edited

Loading

stufreen left a comment

stufreen Feb 22, 2023

kjellr Feb 22, 2023 •

edited

Loading

stufreen Feb 22, 2023

stufreen Feb 22, 2023 •

edited

Loading

kjellr Feb 22, 2023

stufreen Feb 22, 2023

kjellr Feb 22, 2023

ludoboludo left a comment

ludoboludo Feb 22, 2023

kjellr commented Feb 23, 2023

		@@ -1,4 +1,5 @@
		{{ 'section-image-banner.css' \| asset_url \| stylesheet_tag }}
		{{ 'component-rte.css' \| asset_url \| stylesheet_tag }}

Try more universal rich text controls #2085

Try more universal rich text controls #2085

Conversation

kjellr commented Nov 1, 2022 • edited Loading

PR Summary:

Screenshot

Why are these changes introduced?

What approach did you take?

Visual impact on existing themes

Demo links

Decision Log

Checklist

kjellr commented Nov 2, 2022 • edited Loading

elyseviotto commented Nov 8, 2022

ludoboludo commented Feb 15, 2023 • edited Loading

ludoboludo commented Feb 17, 2023

stufreen commented Feb 17, 2023 • edited Loading

kjellr commented Feb 17, 2023

ludoboludo left a comment

Choose a reason for hiding this comment

ludoboludo left a comment

Choose a reason for hiding this comment

kjellr commented Feb 21, 2023 • edited Loading

stufreen left a comment

Choose a reason for hiding this comment

stufreen Feb 22, 2023

Choose a reason for hiding this comment

kjellr Feb 22, 2023 • edited Loading

Choose a reason for hiding this comment

stufreen Feb 22, 2023

Choose a reason for hiding this comment

stufreen Feb 22, 2023 • edited Loading

Choose a reason for hiding this comment

kjellr Feb 22, 2023

Choose a reason for hiding this comment

stufreen Feb 22, 2023

Choose a reason for hiding this comment

kjellr Feb 22, 2023

Choose a reason for hiding this comment

ludoboludo left a comment

Choose a reason for hiding this comment

ludoboludo Feb 22, 2023

Choose a reason for hiding this comment

kjellr commented Feb 23, 2023

kjellr commented Nov 1, 2022 •

edited

Loading

kjellr commented Nov 2, 2022 •

edited

Loading

ludoboludo commented Feb 15, 2023 •

edited

Loading

stufreen commented Feb 17, 2023 •

edited

Loading

kjellr commented Feb 21, 2023 •

edited

Loading

kjellr Feb 22, 2023 •

edited

Loading

stufreen Feb 22, 2023 •

edited

Loading