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

Remove/Replace unordered list tags in the Rich Text Section's Heading Block #2171

Closed
wants to merge 2 commits into from

Conversation

kjellr
Copy link
Contributor

@kjellr kjellr commented Dec 13, 2022

PR Summary:

  • Fixes a bug where unordered lists were allowed in the Rich Text section's Heading block.

Why are these changes introduced?

Fixes #2170

What approach did you take?

The intent of this field is to have it behave like a single-line richtext field, so this PR should strips out the bullets for now.

Before

Screenshot 2022-12-13 at 2 20 45 PM

After

Screenshot 2022-12-13 at 2 19 21 PM

Other considerations

(In the longterm, this richtext field should eventually be replaced with a inline_richtext to prevent these sorts of issues in the future, but for now this specific bug should be fixed.)

Visual impact on existing themes

If they have broken-looking bullets in this heading, the broken-looking bullets will disappear.

Testing steps/scenarios

  1. Visit the demo store.
  2. Select the Heading block within a Rich Text section.
  3. Add an unordered list.
  4. Note that the list tags are reformatted into a <span> upon render.

Checklist

@kjellr kjellr added the Category: Bug Something isn't working label Dec 13, 2022
@kjellr kjellr self-assigned this Dec 13, 2022
@kjellr kjellr marked this pull request as ready for review December 13, 2022 19:42
@@ -26,7 +26,7 @@
{%- case block.type -%}
{%- when 'heading' -%}
<h2 class="rich-text__heading rte {{ block.settings.heading_size }}" {{ block.shopify_attributes }}>
{{ block.settings.heading | replace: 'p>', 'span>' }}
{{ block.settings.heading | replace: 'p>', 'span>' | replace: 'li>', 'span>' | remove: '<ul>' | remove: '</ul>' }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say I'm not for this change. It's visually going to impact a merchant stores and I think it's more a patch of something we should actually fix.

Screenshot

We should change the from richtext to inline-text and either have it part of the next update which can impact merchants manually upgrading to it or we might have a fix in the background that fixes that for them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I totally agree that we should migrate this to inline_richtext. My reasoning for patching this fix now is:

  • In general, it definitely looks like a bug. I don't expect this is what any merchant wants this to look like. Unordered lists are a brand new feature, and this makes the rollout seem unconsidered and unpolished.
  • Even if this were an appearance that the merchant wants, I don't think it makes sense to allow merchants to start using lists here considering our ultimate plans are to switch this to an inline field.

Given that we don't have a definite solution to the richtext > inline_richtext migration issue yet, I think it makes sense to just push this as a really minor temporary fix.

@kjellr
Copy link
Contributor Author

kjellr commented Mar 13, 2023

Closing because #2326 makes this unnecessary. 👍

@kjellr kjellr closed this Mar 13, 2023
@kjellr kjellr deleted the fix-unordered-list-richtext branch March 13, 2023 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Rich text] List element tags aren't stripped out for the Rich Text Section's Heading Block
2 participants