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

Prevent summary list from wrapping mid-word for most browsers #1185

Merged
merged 2 commits into from
Feb 15, 2019

Conversation

MalcolmVonMoJ
Copy link
Contributor

@MalcolmVonMoJ MalcolmVonMoJ commented Feb 14, 2019

As discussed with Colin Rotherham

PR #1169 added in some CSS:

  • word-break: break-word;, followed by
  • word-break: break-all;

word-break: break-word; is the favoured behaviour, but isn't fully supported.
word-break: break-all; is the fallback behaviour, it sometimes results in unfavourable wrapping mid-word.

The order for this CSS needs to be changed to, otherwise the less favourable break-all takes precedence.

word-break: break-all;
word-break: break-word;

caniuse said:

Chrome, Safari and other WebKit/Blink browsers also support the unofficial break-word value

The implication here being that it is not fully supported, so word-break: break-all; is still needed as a fallback.

Below screenshot shows the difference in Google Chrome, note the wrapping of the first column.
image

@NickColley
Copy link
Contributor

NickColley commented Feb 14, 2019

@MoJ-Longbeard happy to accept this but would be great to see examples of the impact of these changes if you can?

We also will need a CHANGELOG entry to describe this fix (make sure to put recognition for yourself as part of the notes too please 👍 )

@MalcolmVonMoJ
Copy link
Contributor Author

@NickColley
Done - I think.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1185 February 14, 2019 13:18 Inactive
CHANGELOG.md Outdated
@@ -22,11 +22,13 @@

🔧 Fixes:

- Pull Request Title goes here
- Update \_summary-list.scss
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Update \_summary-list.scss
- Pull Request Title goes here
Description goes here (optional)
([PR #N](https://github.com/alphagov/govuk-frontend/pull/N))
- Minor CSS code change to prevent wrapping mid-word for most browsers

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 super obvious but we try and keep the template around for contributors in the future to copy and paste.

Also I think the sentence below 'update summary list' is a more descriptive title we can use.

Happy to do this for you when it gets merged, let me know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@NickColley , sure. Please do that for me, happy with the description change. I'm still getting to grips with GitHub etc...

@NickColley NickColley changed the title Update _summary-list.scss Prevent summary list from wrapping mid-word for most browsers Feb 14, 2019
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1185 February 14, 2019 17:47 Inactive
[PR alphagov#1169](alphagov#1169) added in some CSS:
- ``word-break: break-word;``, followed by
- ``word-break: break-all;``

``word-break: break-word;`` is the favoured behaviour, but isn't fully supported.
``word-break: break-all;`` is the fallback behaviour, it sometimes results in unfavourable wrapping mid-word.

The order for this CSS needs to be changed to, otherwise the less favourable `break-all` takes precedence.
```CSS
word-break: break-all;
word-break: break-word;
```

**caniuse** said:
> Chrome, Safari and other WebKit/Blink browsers also support the unofficial break-word value

The implication here being that it is not fully supported, so ``word-break: break-all;`` is still needed as a fallback.

Discussed with [Colin Rotherham](https://github.com/colinrotherham)
@NickColley
Copy link
Contributor

I've cleaned up the branch for you :)

Going to wait for an approval from @colinrotherham since he's been thinking about this a lot too.

@colinrotherham
Copy link
Contributor

For anyone else looking, we originally referred to https://caniuse.com/#search=word-break

Chrome, Safari and other WebKit/Blink browsers also support the unofficial break-word value which is treated like word-wrap: break-word.

And the non-standard comment by MDN:

/* Keyword values */
word-break: normal; 
word-break: break-all; 
word-break: keep-all;
word-break: break-word; /* non-standard */ 

We made sure that the standard is preferred when available.

But… thanks to research by @MoJ-Longbeard we now know that break-word has better wrapping behaviour and is less likely to break up words, unless absolutely necessary.

We also found it's quite clearly listed in the latest CSS Text Module Level 3 working draft from December 2018: https://www.w3.org/TR/css-text-3/#overflow-wrap-property 😊

@NickColley So this is a 👍 from me!

Copy link
Member

@hannalaakso hannalaakso left a comment

Choose a reason for hiding this comment

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

LGTM, thanks all 👏

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