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 word breaks #1271

Merged
merged 3 commits into from
Jan 26, 2022
Merged

Fix word breaks #1271

merged 3 commits into from
Jan 26, 2022

Conversation

KaichenWang
Copy link
Contributor

@KaichenWang KaichenWang commented Jan 26, 2022

Why are these changes introduced?

Mitigate visual issues caused by headings overflowing containers and word breaks

What approach did you take?

1. Remove usage of hyphens CSS property

Some fonts (example: Americana) do not properly support hyphens: auto, resulting in unnecessary breaks in words with no actual hyphen rendered (see screenshot below). Also see #1271 (comment)

Screen Shot 2022-01-26 at 5 23 23 PM2

2. Add word-break: break-word; for all headings

This prevents headings (which are typically set to larger font-sizes) from overflowing containers.

3. Change collection card headings to H3 style instead of H1 style

Reduce likelihood of text overflowing containers by reducing font-size.

Other considerations

Demo links

Checklist

@KaichenWang
Copy link
Contributor Author

Just want to note that @melissaperreault noticed that buttons (example: rich text) can also have issues with longer labels:

22-01-ir55g-13tmq

@melissaperreault melissaperreault self-requested a review January 26, 2022 21:29
Copy link
Contributor

@tyleralsbury tyleralsbury left a comment

Choose a reason for hiding this comment

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

Tested headings for all sections, including content based ones in blocks like Image Banner, etc...

Also tested all main pages - product, collection, blog, article, etc...

Everything works well with regards to headings. Far as I can tell, we are good.

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.

Thanks you Kai!!

Everything looks good except the Collage edge-case findings and other areas we could benefit from word-break as a quick follow up. I tested all instances a label could have a very long word.

Edge-case

Other text findings

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.

👍

@@ -311,13 +311,11 @@

.image-with-text__heading {
word-break: break-word;
hyphens: auto;
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove usage of hyphens CSS property

Can you share a bit more context on the decision to remove hyphens? I probably missed this conversation, but would love to know the rationale behind this so we can have it documented :D

Copy link
Contributor

Choose a reason for hiding this comment

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

Hyphens are very unpredictable and override any other wrapping behaviours we try to apply. Word break, overflow break, etc... - they are all overridden by the hyphen rules. Unfortunately, on top of that, the rules are independent of whether or not the font actually supports hyphens. So you can get situations where the hyphens are breaking something that wouldn't otherwise be broken, and which would look okay with a - character, but where the - is not being drawn because of the font.

So to make it so that everything is predictable and all works the same, we are using word break. We will be revisiting this to instead use overflow-wrap: break-word, most likely, but that requires some additional work and testing. Also we need to consider what we want to do for other things like paragraphs and buttons.

It might just end up being a situation where we add a setting for the merchant to pick how they want this handled, especially to account for different expectations in different languages. Though things do get interesting with stores that multi-language, since settings aren't language specific.

@KaichenWang KaichenWang merged commit bafe4f3 into main Jan 26, 2022
@KaichenWang KaichenWang deleted the fix-word-breaks branch January 26, 2022 22:42
@KaichenWang
Copy link
Contributor Author

@melissaperreault Created follow up issue for items you noted. #1273

phapsidesGT pushed a commit to Gravytrain-UK/gt-shopify-dawn-theme that referenced this pull request Sep 3, 2024
* Add word-break to headings

* Remove usage of hyphens css property due to unpredictability

* Change collection card heading to h3 style instead of h1
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.

5 participants