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

Changed deprecated word-break: break-word to overflow-wrap #410

Merged
merged 1 commit into from
Mar 14, 2022

Conversation

jsines
Copy link
Contributor

@jsines jsines commented Feb 15, 2022

Types of change

  • Bug fix (change which fixes an issue)
  • Enhancement (change which improves upon an existing feature)
  • New feature (change which adds new functionality)

Description

Removes use of deprecated word-break: break-word and switches to overflow-wrap: break-word.

Checklist

People to notify

CC: @jasonnrosa

@jsines jsines requested a review from jasonnrosa February 15, 2022 16:46
@jsines jsines merged commit a451d7d into main Mar 14, 2022
@jsines jsines deleted the remove-deprecated-css branch March 14, 2022 13:39
@JacobLett
Copy link

I have had to remove this property on a few sites because it was causing individual characters breaking to a new line. For example.

Finall
y

Is this added just to prevent horizontal scrolling from long strings of text? I personally think this should be removed.

@jasonnrosa
Copy link
Member

Hey, @JacobLett that is correct - it is added to prevent a break in responsiveness on the page in the event that an individual word overflows out of the normal boundary of the page. This is something we've had customers reach out about prior to having a solution in place which is why it has been implemented. Overall we've found that the issues of having the horizontal scroll outweigh the line wrapping from a user experience perspective as the horizontal scroll can make the page harder to navigate. This is largely here as a fallback for extreme scenarios with long words or large font sizes, but I think generally, the preferred solution is to reduce the font size on mobile to allow for words to fit better on the page.

Do you have any scenarios where removing this helped (but didn't break responsiveness/create horizontal scroll on the page)?

@JacobLett
Copy link

@jasonnrosa ok that makes sense then. I think if a company doesn't have developer support this is good. If this setting is turned off, it requires a developer to decrease the font sizes like you mentioned when appropriate.

Most of the time I removed this and decreased font sizes on mobile. Spot-checked them for areas that were breaking layout.

@johnsfuller
Copy link
Contributor

I'll also chime in here... I think choosing between text break vs non text break in large font mobile scenarios is like choosing between mold or mildew for dinner – neither is desirable.

Most clients' web traffic is now 50% or more for mobile. I think it's time to add mobile text size fields.

@jasonnrosa
Copy link
Member

Thank you both!

@JacobLett agreed on the preferred solution there - I think we mainly have this as a fallback just in case. E.g. boilerplate is used as a starting point for a lot of marketplace themes that might not have as much interaction with a customer as opposed to a 1:1 with a developer/agency (and even if there were more granular font options there is always a chance a user doesn't use them correctly). Do you think it is worth adding a comment in the style sheet that notes that this is some optional CSS and why it is in place, so a dev going to boilerplate for the first time can remove it if they prefer?

@johnsfuller yes agreed I think both are not great solutions (horizontal scroll or word wrap) but as mentioned above the CSS above is largely just a fallback for extreme scenarios for the preferred of the two experiences (I think seeing a piece of wrapped text is not great but I think navigating a site that scrolls horizontally is much more noticeable/a worse experience).

With that being said, fully agreed that mobile options are the best path forward here but it is something we want to think on a little bit more to get correct from a CMS perspective (e.g. determine if this should be more of a built-in feature in theme settings vs. adding additional fields) before we officially add it as a recommendation here to boilerplate.

Out of curiosity, how do you currently handle mobile options for theme settings? Do you offer a setting for each spot where font sizes can be changed in the theme right now or do you have a single setting that can reduce all font sizes on mobile X % (or something entirely different)?

@JacobLett
Copy link

I seem to run into this with hero banners and other sections that use headings that are bigger then the default h1. Like display text. I would then add a field to specify a mobile % of the default in that module. So it is a case by case edit.

I would be in favor of having a note in the CSS just to say this is a fallback and you may want to remove this and solve for mobile heading sizes.

I started questioning this because I wanted to know the rationale behind the setting since I kept on removing it. "That's the nature of the web baby. You can't control everything." *(single tear)

@johnsfuller
Copy link
Contributor

We have a select option to do one of the following

  1. automatically make heading fonts smaller on mobile (works most of the time)
  2. give users individual control over heading mobile font sizes
  3. do nothing

@jasonnrosa
Copy link
Member

Thank you both for the insight into how you're currently solving this - that is very helpful! I like the idea of an automatic option but also allowing users to override more granularly where they want and/or allowing for a percentage reduction on mobile. I spun up an issue here for tracking adding this to boilerplate and will add a comment per your suggestion @JacobLett to the CSS file in the interim so the purpose is clearer.

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.

4 participants