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

Fix double margins in RTE when non-paragraphs are used #2403

Merged
merged 2 commits into from
Apr 19, 2023

Conversation

kjellr
Copy link
Contributor

@kjellr kjellr commented Mar 15, 2023

PR Summary:

Fixes unintentional double margins generated by non-paragraph tags in the Rich Text editor.

Why are these changes introduced?

Fixes #2416

Recent updates to the Rich Text editor now allow it to use headings and lists, in addition to paragraphs. Dawn today clears out the top and bottom margins of paragraphs inside of the Rich Text editor. These new tags were not targeted by our existing rule, resulting in more vertical space between elements than we'd originally designed for there to be.

What approach did you take?

I used a wildcard here — I can't think of a reason why we wouldn't want to clear these extra margins. But if folks feel otherwise, it's easy enough to just add a bunch of rules for all the currently-supported tags instead (.rte > h1:first-child, .rte > h2:first-child, etc.). The downside is just that it's far more code, and could break again if we allow more tags in here in the future.

Visual impact on existing themes

Paragraph Tags (No Change)

Before After
both-p both-p

H2 Tags

Before After
before-h2 after-h2

H3 Tags

Before After
before-h3 after-h3

This also effects ul, ol, and h1-h6, but the effect is the same.

Testing steps/scenarios

  1. Open the demo store.
  2. Add an image with Text Block
  3. Edit the description text style so that it uses headings or lists.
  4. Note that the vertical margins between the heading + description + button stay consistent.
  5. Check other uses of rte and look for visual changes. (i.e. product descriptions, blog posts, and pages). There should be no visual change when this PR is active.

Demo links

Checklist

@kjellr kjellr added the Category: Bug Something isn't working label Mar 15, 2023
@kjellr kjellr self-assigned this Mar 15, 2023
@stufreen
Copy link
Contributor

In some templates we use the rte class for product, page, and blog descriptions. These have a different rich text editor in admin which lets you put in images and stuff. This change is probably still fine but we should test these separately.

E.g.

<div class="product__description rte quick-add-hidden" {{ block.shopify_attributes }}>

<div class="rte">

https://github.com/Shopify/dawn/blob/main/sections/main-article.liquid#L59

@kjellr
Copy link
Contributor Author

kjellr commented Mar 15, 2023

@stufreen Thanks! I just double checked as many of those use cases as I could think of, and I'm not seeing any visual changes after the rule is applied. I'll add a note to the testing instructions for other folks to look too though. 👍

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.

Tested every section/template I saw that used the .rte class and it all seem good 👍

@kjellr kjellr marked this pull request as ready for review March 15, 2023 18:39
@kjellr
Copy link
Contributor Author

kjellr commented Apr 12, 2023

Whoops, I forgot about this one for a while. 😅 @stufreen do you have a minute for a second review?

@@ -3157,11 +3157,11 @@ details-disclosure > details {
display: block;
}

.rte > p:first-child {
.rte > *:first-child {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not a blocker but just calling out that this issue didn't really occur in the Richtext section because the styling there is different. There is a rule in the richtext heading block to remove the bottom margin.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also just a random non-blocker: On the Page template the first child is actually a meta element. So the rule is not doing anything there.

Screenshot 2023-04-19 at 2 55 33 PM

Copy link
Contributor

@ludoboludo ludoboludo Apr 19, 2023

Choose a reason for hiding this comment

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

For the page I think it depends. Sometimes things get copied over in the rich text editor and it pastes in some unwanted elements like this.
If you look at some other pages like a complete page, it's not there. So it's not added by the RTE from the admin but more so due to the way content can be added I believe 🤔

@stufreen stufreen merged commit f5ed284 into main Apr 19, 2023
@stufreen stufreen deleted the fix-rte-double-margins branch April 19, 2023 20:50
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
Category: Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RTE Behaviour] Issues with spacing when customizing headlines
3 participants