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] Improve list alignment #2185

Merged
merged 3 commits into from
Jan 13, 2023
Merged

[Rich text] Improve list alignment #2185

merged 3 commits into from
Jan 13, 2023

Conversation

andrewetchen
Copy link
Contributor

PR Summary

This PR improves the alignment of unordered and ordered rte lists.

Why are these changes introduced?

Fixes #2155

What approach did you take?

  1. I added list-style-position: inside; to .rte ul and .rte ol.
  2. I also opted to add the rte class to the Featured collection section > collection__description so that that it would inherit the same alignment.

Other considerations

Unordered lists were recently introduced as a new richtext feature. I included ordered lists in the event that we add ordered lists to the richtext input setting. Additionally, accounting for ordered lists now also improves the ordered lists on the product and collection description rich text.

Visual impact on existing themes

The alignment of currently used rich text ul and ol lists will be improved.

Screenshots 🖼️

Rich text section - before

Desktop

Content alignment: left

image

Content alignment: center

image

Content alignment: right

image

Mobile

Content alignment: left

image

Content alignment: center

image

Content alignment: right

image

Rich text section - after

Desktop

Content alignment: left

image

Content alignment: center

image

Content alignment: right

image

Mobile

Content alignment: left

image

Content alignment: center

image

Content alignment: right

image

Testing steps/scenarios 🧪

To help with side-by-side visual comparisons, I'd recommend opening a separate browser window with the latest version of Dawn main: https://os2-demo.myshopify.com/admin/themes/127903858710/editor

  • Rich text section: Add Heading and Text blocks and change them to unordered lists
    • Test the different "Content alignment" settings to visually compare against main
  • Other sections: There are other sections that have a richtext setting. You can also check those sections to visually compare their unordered lists alignment
    • Collapsible content > Collapsible row
    • Email signup banner > Subheading
    • Featured collection > Description (Toggle the Show collection description from the admin setting)
    • Footer > Text
    • Image with text > Text
    • Multicolumn > Column > Description
    • Password > Email signup banner > Paragraph
  • Product and collection rte descriptions
    • Check the No featured image product description
    • Check the Shoes collection description

Demo links 🔗

Checklist

@@ -55,7 +55,7 @@
or section.settings.show_description
and section.settings.collection.description != empty
-%}
<div class="collection__description {{ section.settings.description_style }}">
<div class="collection__description {{ section.settings.description_style }} rte">
Copy link
Contributor

Choose a reason for hiding this comment

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

Kinda similar to my first comment, here we will need to call the component-rte.css file to be sure the styling is applied.
But maybe we move RTE styles to base.css instead. I'm not sure what's better. Calling multiple assets or having a huge file tbh.
Maybe for now we keep it as is and call that css file from the top of this file. But that could be an asset we load in theme.liquid as rte settings or rte from the admin is most likely on every page of the theme 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm leaning towards calling multiple assets but keeping the base file clean.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I'm in favour of keeping things as is for now and just adding the css file to this section. I've added component-rte.css to the featured collection section here: 7c52fc2

My plan was to improve what we can now as a quick fix/win, then revisit this again when new rte features start to roll in. I agree that we can probably improve things further by moving everything from component-rte.css to base.css or as you've suggested, we could just add component-rte.css to theme.liquid so that it wouldn't have to be declared in each of those section files. I'll make a note of this.

eugenekasimov
eugenekasimov previously approved these changes Dec 22, 2022
Copy link
Contributor

@ludoboludo ludoboludo left a comment

Choose a reason for hiding this comment

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

I think there are a couple files where we will need to call the component-rte.css asset:

  • collapsible-content.liquid
  • image-with-text.liquid
  • multicolumn.liquid
  • newsletter.liquid

Otherwise everything seemed good 👍

@andrewetchen
Copy link
Contributor Author

I think there are a couple files where we will need to call the component-rte.css asset:

  • collapsible-content.liquid
  • image-with-text.liquid
  • multicolumn.liquid
  • newsletter.liquid

Thanks Ludo.

I can see why this was never flagged before (some sections not loading component-rte.css while using the rte class) -- other sections (in view) must've already loaded it to even notice. All the more reason to revisit the loading strategy as discussed here: #2185 (comment) 👍🏼

I've added the necessary changes here: 72523a1, all of these sections should now be loading the component-rte.css asset

@melissaperreault
Copy link
Contributor

Thank you Andrew, I took a quick quick look and need more time to review.

  • One question I had was if we can control the margin between the bullet and the first letter. It looks a little far away in general. We can pair to discuss what I mean.
  • There is the subsequent line that we would need to polish too, so it can follow the indentation as well.

@andrewetchen andrewetchen self-assigned this Jan 9, 2023
@andrewetchen
Copy link
Contributor Author

Thanks for taking a look, Melissa.

One question I had was if we can control the margin between the bullet and the first letter. It looks a little far away in general. We can pair to discuss what I mean.

I see what you mean and I agree that it could be a little closer. Unfortunately, there's currently not many allowable properties that we can use for the ::marker pseudo-element, including margin and padding.

There is the subsequent line that we would need to polish too, so it can follow the indentation as well.

I agree and something else I noticed while working on a potential indentation fix was that it renders differently on Chrome, Firefox, and Safari using the following CSS (subtle but still noticeable):

CSS (initial indentation fix)
  • I added this to assets/component-rte.css

    .rte ul,
    .rte ol {
      list-style-position: inside;
      padding-left: 2rem;
    }
    
    .rte li {
      padding-left: 2.75rem;
      text-indent: -2.05rem;
    }
    
    @media screen and (min-width: 750px) {
      .rte li {
        text-indent: -2.2rem;
      }
    }
Screenshot (Chrome, Firefox, Safari)

image

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! Let's follow up on the other issue! 🚢

@andrewetchen andrewetchen merged commit 97e1714 into main Jan 13, 2023
@andrewetchen andrewetchen deleted the rte-lists-alignment branch January 13, 2023 21:39
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.

[Rich text] List element alignment isn't pretty
4 participants