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

Improve word wrapping in summary list component #1220

Merged
merged 3 commits into from
Mar 1, 2019

Conversation

NickColley
Copy link
Contributor

@NickColley NickColley commented Feb 22, 2019

This seems like a problem where we have to make some compromises.

The summary list currently wraps text in all browsers, but only in blink/webkit does it wrap entire words, all other browsers it can wrap mid word which can make it more difficult to read.

To make this better we want to try and use overflow-wrap which works cross browsers, however...

In order to use overflow-wrap with table CSS we need to use a fixed layout since the summary list uses table CSS.

This means we can't rely on a table's automatic resizing, so we have to compromise
with a larger actions column that we have before, so that we can have at least two actions on one line without the actions wrapping.

Welcome any input to an alternative that I may have missed.

I have also considered that this might be best solved with CSS Grid but have discounted that for now based on the support for browsers.

You can take a look at the two by comparing:

Before

Screenshots

Without changes there's no gap

After

Pros

  • Works more consistently across all browsers we support

Cons

  • Content cannot decide the size of columns anymore leading to smaller columns than before

Screenshots

With changes, there's a gap between columns

Related links

Aims to resolve #1211

Use readable English to better highlight any issues with wrapping
Check how wrapping behaves in the context we expect.
In order to use overflow-wrap with table CSS we need to use a fixed layout.

This means we can't rely on a table's automatic resizing, so we have to compromise
with a larger 'actions' column that we have before.
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1220 February 22, 2019 15:28 Inactive
@NickColley NickColley added this to the 2.8.0 milestone Feb 25, 2019
Copy link
Contributor

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

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

Generally looks a lot better to me 👍

Improvements most obvious in IE, Edge and Firefox.

Chrome 70 (macOS)

Before
macmo_chrome_70 0-before

After
macmo_chrome_70 0-fixed

Chrome 71 (macOS)

Before
macmo_chrome_71 0-before

After
macmo_chrome_71 0-fixed

Firefox 62 (macOS)

Before
macmo_firefox_62 0-before

After
macmo_firefox_62 0-fixed

Safari 12 (macOS)

Before
macmo_safari_12 0-before

After
macmo_safari_12 0-fixed

IE 8 (Windows 7)

Before
win7_ie_8 0-before

After
win7_ie_8 0-fixed

IE 9 (Windows 7)

Before
win7_ie_9 0-before

After
win7_ie_9 0-fixed

IE 10 (Windows 8)

Before
win8_ie_10 0-before

After
win8_ie_10 0-fixed

IE 11 (Windows 10)

Before
win10_ie_11 0-before

After
win10_ie_11 0-fixed

Chrome 71 (Windows 10)

Before
win10_chrome_71 0-before

After
win10_chrome_71 0-fixed

Chrome 70 (Windows 10)

Before
win10_chrome_70 0-before

After
win10_chrome_70 0-fixed

Edge 18 (Windows 10)

Before
win10_edge_18 0-before

After
win10_edge_18 0-fixed

Firefox 62 (Windows 10)

Before
win10_firefox_62 0-before

After
win10_firefox_62 0-fixed

Android 7.1 (Google Pixel)

Before
7 1_google-pixel_portrait_real-mobile-before

After
7 1_google-pixel_portrait_real-mobile-fixed

Android 8.0 (Google Pixel 2)

Before
8 0_google-pixel-2_portrait_real-mobile-before

After
8 0_google-pixel-2_portrait_real-mobile-fixed

Android 9.0 (Google Pixel 2)

Before
9 0_google-pixel-2_portrait_real-mobile-before

After
9 0_google-pixel-2_portrait_real-mobile-fixed

iOS 10 (iPhone 7)

Before
10_iphone-7_portrait_real-mobile-before

After
10_iphone-7_portrait_real-mobile-fixed

iOS 11 (iPhone 8)

Before
11_iphone-8_portrait_real-mobile-before

After
11_iphone-8_portrait_real-mobile-fixed

iOS 10 (iPhone XR)

Before
12_iphone-xr_portrait_real-mobile-before

After
12_iphone-xr_portrait_real-mobile-fixed

@NickColley
Copy link
Contributor Author

I got an approval from @dashouse over Slack for this so I'm going to push forwards and get it merged thanks again for the great review @36degrees

@NickColley NickColley merged commit 4446431 into master Mar 1, 2019
@NickColley
Copy link
Contributor Author

Need to add a CHANGELOG entry for this, will follow up in a second.

@NickColley NickColley deleted the improve-word-wrapping branch March 1, 2019 14:58
@36degrees 36degrees mentioned this pull request Mar 5, 2019
chrimesdev added a commit to nhsuk/nhsuk-frontend that referenced this pull request Mar 11, 2019
Following the improvements to the GOV.UK frontend summary list
component alphagov/govuk-frontend#1220 these
changes have been applied.
chrimesdev added a commit to nhsuk/nhsuk-frontend that referenced this pull request Mar 11, 2019
Following the improvements to the GOV.UK frontend summary list
component alphagov/govuk-frontend#1220 these
changes have been applied.
chrimesdev added a commit to nhsuk/nhsuk-frontend that referenced this pull request Mar 11, 2019
Following the improvements to the GOV.UK frontend summary list
component alphagov/govuk-frontend#1220 these
changes have been applied.
chrimesdev added a commit to nhsuk/nhsuk-frontend that referenced this pull request Mar 11, 2019
Following the improvements to the GOV.UK frontend summary list
component alphagov/govuk-frontend#1220 these
changes have been applied.
chrimesdev added a commit to nhsuk/nhsuk-frontend that referenced this pull request Mar 11, 2019
Following the improvements to the GOV.UK frontend summary list
component alphagov/govuk-frontend#1220 these
changes have been applied.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Revert fix for wrapping of long lines of text
3 participants