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 wrapping of long lines of text in summary list #1169

Merged
merged 1 commit into from
Feb 4, 2019

Conversation

colinrotherham
Copy link
Contributor

@colinrotherham colinrotherham commented Jan 31, 2019

As raised by @MoJ-Longbeard, we noticed very long links or lines of text aren't automatically wrapped inside the summary list value (but they are in the summary list key).

This pull request adds automatic wrapping to both key and value.

Before

Shows the summary list scrolling off the screen

before

After

Shows how text is automatically wrapped, only if it's too long to fit

after

For compatibility with other browsers, we need to provide multiple values for word-break:

.govuk-summary-list__key,
.govuk-summary-list__value {
  // sass-lint:disable no-duplicate-properties
  // Automatic wrapping for unbreakable text (e.g. URLs)
  word-break: break-word; // WebKit/Blink only
  word-break: break-all; // Standards
}

@colinrotherham
Copy link
Contributor Author

Is there a better way to guess the next pull request ID in GitHub for the changelog?

Fixed it and pushed again.

@NickColley
Copy link
Contributor

@colinrotherham you could guess what the next ID is by bumping the number, but we just open the PR then add a CHANGELOG entry...

Copy link
Contributor

@NickColley NickColley left a comment

Choose a reason for hiding this comment

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

This looks great to me!

I've tested this cross browser too.

Thanks for the great work updating the example Colin.

@MoJ-Longbeard are you happy with this? 😄

We'll need a section person from the team to also check and approve this.

@MalcolmVonMoJ
Copy link
Contributor

@NickColley, Apologies if I am being too pernickety, but I am slightly concerned that this CSS deactivates the table's "natural" resizing.
I applied these changes to the example here and the email address (sarah.phillips@example.com) wraps where it wouldn't have caused an issue before. The issue seems to be the same as my screenshot posted in Slack (at 2019-01-31, 2:59 PM).

@colinrotherham
Copy link
Contributor Author

colinrotherham commented Feb 1, 2019

@MoJ-Longbeard Yeah you're right. Browsers definitely aren't fantastic at dealing with it.

Without this change it's already affecting key names at narrow sizes:

chopped

Let me see if a display: block wrapper and overflow-wrap: break-word; would work better, it seems to be a nicer implementation.

@colinrotherham
Copy link
Contributor Author

@MoJ-Longbeard I tried that option, but can't find a better solution.

I'd say that wrapping by default is sensible, but manually choosing to add white-space: nowrap; for cases where wrapping should absolutely not happen (like your Sarah Phillips example).

I've added another example to this pull request:

<p class="govuk-body" style="white-space: nowrap;">
    michelle.longish.name@example.com
</p>

The Summary List "extreme" example now demonstrates text wrapping for the two new long rows, but never goes narrower than the width of this paragraph.

@NickColley
Copy link
Contributor

Just a note, the examples in the design system iframes are a little squished than they would be in real life, if you 'open in a new window' you'll see the actual result.

@colinrotherham
Copy link
Contributor Author

Quickly modified to list the non-standard break-word before break-all, as is best practice

@NickColley
Copy link
Contributor

Spoke with @MoJ-Longbeard and @colinrotherham on Slack.

The current situation is:

  1. If we break words, it can break words unnecessarily
  2. If we don't break words, it can overflow

So there's no perfect solution, without perhaps using JavaScript, which we will not want to do.

We think that since super long words are edge cases, this fix will not break the majority of content in summary lists unnecessarily.

@MalcolmVonMoJ
Copy link
Contributor

Yes. I agree. As you say, neither option is perfect.
I agree that it will just be for emails and URLs, not actual words, and I would think that a wrapped email address or URL will attract fewer bug reports than one that overflows the edge of the page.
I understand that the longest non-technical English word has 29 letters, so I think we are safe.

@dashouse dashouse self-requested a review February 4, 2019 12:09
@NickColley NickColley merged commit affd1fa into alphagov:master Feb 4, 2019
@NickColley
Copy link
Contributor

Thanks everyone, @MoJ-Longbeard let us know how soon you need a release for this. We tend to wait a bit between releases but can prioritise a quick release if you are blocked. 👍

@colinrotherham colinrotherham deleted the summary-list-wrapping branch February 5, 2019 09:10
@aliuk2012 aliuk2012 mentioned this pull request Feb 8, 2019
@aliuk2012 aliuk2012 added this to the v2.7.0 milestone Feb 11, 2019
MalcolmVonMoJ added a commit to MalcolmVonMoJ/govuk-frontend that referenced this pull request Feb 14, 2019
[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;
```
MalcolmVonMoJ added a commit to MalcolmVonMoJ/govuk-frontend that referenced this pull request Feb 14, 2019
[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 pushed a commit to MalcolmVonMoJ/govuk-frontend that referenced this pull request Feb 14, 2019
[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)
@leekowalkowski-hmrc
Copy link

I think we should use overflow-wrap.

word-break: https://jsfiddle.net/lee_kowalkowski/efvb7uxm/3/
overflow-wrap: https://jsfiddle.net/lee_kowalkowski/efvb7uxm/6/

@NickColley
Copy link
Contributor

@leekowalkowski-hmrc looks like that might be a good option, do we know how well it works with legacy browsers? https://caniuse.com/#search=overflow-wrap

@leekowalkowski-hmrc
Copy link

@NickColley Yes, it looks like we need word-wrap: break-word, too:

word-wrap: break-word;
overflow-wrap: break-word;

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.

6 participants