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

[Rich text] Change rich text section heading type #2326

Merged
merged 1 commit into from
Feb 28, 2023

Conversation

ludoboludo
Copy link
Contributor

@ludoboludo ludoboludo commented Feb 23, 2023

PR Summary:

Change the richtext section heading block type to be an inline_richtext type instead of richtext.

Move the component-rte.css styling into base.css since its loaded on every page.

Fixes #2209

Why are these changes introduced?

Continue the work done by @kjellr so that most headings in our theme are using this type.

Visual impact on existing themes

There is a visual change expected for merchants that used the richtext specific features in their heading but we're planning to do some backend work to limit the loss of their content when upgrading to the next theme version.
Though some content can still be lost like metafields

Testing steps/scenarios

  • Check out the richtext section and make sure it's an inline_richtext allowing you to only do bold, italic and links.
  • Make sure there isn't any other place in which content should have been tweaked.

⚠️ There has been some formatting changes due to our liquid prettier. They don't change anything but the format only

Demo links

Checklist

product: product,
product_form_id: product_form_id,
section_id: section.id
-%}
Copy link
Contributor

Choose a reason for hiding this comment

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

There are a bunch of little reformatting additions like these that make it harder to review. None of it appears to actually change anything though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah 😞 I will add a note in the PR description to mention this. We're trying to default to use our own prettier extension and I guess some stuff wasn't formatted in some files.

@kjellr
Copy link
Contributor

kjellr commented Feb 27, 2023

Thanks for following through on this. It's working well in my testing! It's nice to eliminate all the extra component-richtext.css calls.

I left one comment inline, and have just a single followup question here:

some content can still be lost like metafields

When this happens do we/can we do anything to alert the user? Or can we at least warn them up front before upgrade?

@ludoboludo
Copy link
Contributor Author

When this happens do we/can we do anything to alert the user? Or can we at least warn them up front before upgrade?

We will have to make sure the next release or the release it's part of is one where we don't force push the update/changes. Manual update means there are visual/breaking changes, so historically it's always meant having to double check your theme to make sure everything looks ok.

So far we don't have a lot of ways to warn the merchants but through our release notes.
I doubt many merchants check those though. It'd be nice if we could share them/make them accessible in the theme editor.

@kmeleta kmeleta self-requested a review February 28, 2023 15:50
@kmeleta kmeleta self-assigned this Feb 28, 2023
@kjellr
Copy link
Contributor

kjellr commented Feb 28, 2023

Makes sense. Out of curiosity, do we have any idea how many merchants use meta objects inside those fields today? I wonder how many folks this might effect.

Copy link
Contributor

@melissaperreault melissaperreault left a comment

Choose a reason for hiding this comment

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

Thank you Ludo! Works well! 🚢

Copy link
Contributor

@kmeleta kmeleta left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Contributor

@eugenekasimov eugenekasimov left a comment

Choose a reason for hiding this comment

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

🚀

@ludoboludo ludoboludo merged commit 5f13b1b into main Feb 28, 2023
@ludoboludo ludoboludo deleted the richtext-heading-type-change branch February 28, 2023 19:51
pangloss added a commit to pangloss/dawn that referenced this pull request Mar 11, 2023
* shopify/main:
  Improve image sizes in the multicolumn section (Shopify#2349)
  Fix the Page section's width.  (Shopify#2364)
  Update 12 translation files (Shopify#2366)
  Removing "my" from cart popup notification (Shopify#2353)
  [Cart.js] Fix fetch url so it's not hard coded (Shopify#2357) (Shopify#2365)
  Update 1 translation file (Shopify#2352)
  Default Follow on Shop to on
  [Header] Add localization selectors (Shopify#2258)
  Remove async CSS pattern where it may introduce layout shifts (Shopify#2270)
  Change rich text section heading to be of type inline_richtext, also moved rte styling into base.css (Shopify#2326)
  Add drawer menu desktop (Shopify#2195)
  Make header image preload and proper width (Shopify#2307)
phapsidesGT pushed a commit to Gravytrain-UK/gt-shopify-dawn-theme that referenced this pull request Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve loading of rte styles
5 participants