Don't output an empty ul if 0 items provided #1259

edwardhorsford · 2019-03-28T13:03:23Z

I need to provide an empty action option so that I don't get weird gaps like this:

You can fix this by providing an empty action.items - but right now this results in an empty ul. This pr doesn't spit out the ul if there's 0 items.

NB: untested!

timpaul · 2019-04-03T13:41:19Z

Thanks for this Ed. Looks good, but we'll need to write some tests for it before we merge.

colinrotherham · 2019-04-05T16:12:00Z

Hi @timpaul I've added some tests for @edwardhorsford as one of "Check answers" pages has a row without an actions column and this will affect us too—might as well help out!

This includes a few other changes as well:

1. Allows custom classes on entire rows
To support custom styling of whole rows at a time

2. Fixes the 1px row height change when borders are removed
i.e. Uses transparent border-color rather than removing it entirely:

Before
This removes 1px from each summary list row

border: 0;

After
The height of the row stays the same

border-bottom-color: transparent;

3. Fixes the text alignment when the actions column isn't added
See below, as the 30% and 50% column widths didn't add up to 100%

36degrees · 2019-04-24T10:02:28Z

2. Fixes the 1px row height change when borders are removed
i.e. Uses transparent border-color rather than removing it entirely:

Before
This removes 1px from each summary list row
border: 0;
After
The height of the row stays the same
border-bottom-color: transparent;

This'll affect anyone using custom colours in their browser – they'll see a border when others don't. What's the impact of the 1px smaller row height?

colinrotherham · 2019-04-24T11:52:47Z

@36degrees Impact was cosmetic. I noticed the jump in height with/without borders.

Hypothetically, two summary lists in left and right columns (one with borders, one without) won't be aligned. They'll be out by 1px for each row.

I can roll this back or use an !important or another suggestion?

Thanks

36degrees · 2019-04-24T12:41:02Z

As far as I'm aware, there's no way to stop borders from becoming opaque when colours are overridden, even using !important.

This screenshot illustrates the problem:

I think in this case the best thing to do is roll back that change until we can find a solution that works for everyone, and we can get the rest of these changes merged.

Ta!

36degrees · 2019-04-24T12:43:57Z

src/components/summary-list/template.njk

+{# Determine if we need 2 or 3 columns #}
+{% set hasActions = false %}
+{% for row in params.rows %}
+  {% set hasActions = row.actions.items if row.actions.items else hasActions %}


As this is a boolean, should we set it to true if row.actions.items, rather than the contents of row.actions.items?

Thanks, sorted

36degrees · 2019-04-24T12:44:31Z

src/components/summary-list/template.njk

        <dd class="govuk-summary-list__actions {%- if row.actions.classes %} {{ row.actions.classes }}{% endif %}">
          {% if row.actions.items.length == 1 %}
            {{ _actionLink(row.actions.items[0]) | indent(12) | trim }}
-          {% else %}
+          {% elseif row.actions.items.length > 1 %}


What else could it be, if row.actions.items.length is truthy and it's not 1?

You're right, this got refactored slightly higher up so can go now.

Was part of the first commit: f0df010

36degrees · 2019-04-24T12:45:43Z

src/components/summary-list/template.njk

@@ -6,20 +6,27 @@
    {% endif -%}
  </a>
 {% endmacro -%}
+
+{# Determine if we need 2 or 3 columns #}
+{% set hasActions = false %}


I wonder if this could be clearer, perhaps something like

Suggested change

{% set hasActions = false %}

{% set anyRowHasActions = false %}

Yep I'm happy with this

36degrees · 2019-04-24T12:47:03Z

src/components/summary-list/template.test.js

+
+        expect($action.length).toEqual(0)
+      })
+      it('adds dummy action columns when only some rows have actions', async () => {


36degrees · 2019-04-24T12:48:18Z

src/components/summary-list/_summary-list.scss

@@ -80,6 +80,12 @@
    }
  }

+  .govuk-summary-list__value:last-child {


This could do with a comment I think

colinrotherham · 2019-04-24T15:45:34Z

@36degrees Regarding the borders, usefully each cell has 10px bottom padding.

So with borders turned off, could this be the fix? i.e. Add 1px back on:

.govuk-summary-list__key,
.govuk-summary-list__value,
.govuk-summary-list__actions {
  padding-bottom: govuk-spacing(2) + 1px;
  border: 0;
}

Also fixes the 1px row height jump when borders are removed

36degrees · 2019-04-24T16:03:16Z

Makes sense to me 👍

colinrotherham · 2019-04-24T16:06:09Z

@36degrees Review comments all addressed. Thanks again.

On mobile "no-border" has margin collapsing between the rows, so I've not addressed the height jump here as I imagine that's intentional.

36degrees · 2019-04-24T16:37:06Z

This is looking good I think, thanks again.

It just needs documenting in the changelog and then I can approve it.

colinrotherham · 2019-04-24T16:50:14Z

@36degrees Thanks.

Changelog added, split into both fixes/features.

36degrees

👏

aliuk2012

Thanks @edwardhorsford & @colinrotherham 👏

- Add new secondary and warning button variants ([PR #1207](alphagov/govuk-frontend#1207)) - Add new govuk-shade and govuk-tint functions for creating shades and tints of colours. ([PR #1207](alphagov/govuk-frontend#1207)) - Add support for custom row classes on the summary list component (including support for some rows without action links) ([PR #1259](alphagov/govuk-frontend#1259)) - Ensure fieldset never exceeds max-width This fix ensures that both WebKit/Blink and Firefox are prevented from expanding their fieldset widths to the content's minimum size. This was preventing `max-width: 100%` from being applied to select menus inside a fieldset. See discussion in ["Reset your fieldset"](https://thatemil.com/blog/2015/01/03/reset-your-fieldset/) and raised by [issue #1264](alphagov/govuk-frontend#1264) ([PR #1269](alphagov/govuk-frontend#1269)) - Add various fixes to the summary list component: 1. Fixes the 1px row height change when borders are removed Padding is now adjusted by 1px instead 2. Fixes the text alignment when the actions column isn't added So the key column always stays at 30% width ([PR #1259](alphagov/govuk-frontend#1259)) https://github.com/alphagov/govuk-frontend/releases/tag/v2.11.0

edwardhorsford mentioned this pull request Mar 28, 2019

Summary list alphagov/govuk-design-system-backlog#182

Open

dashouse added the awaiting triage Needs triaging by team label Apr 3, 2019

kellylee-gds added 🕔 hours A well understood issue which we expect to take less than a day to resolve. Priority: Low and removed awaiting triage Needs triaging by team labels Apr 3, 2019

colinrotherham force-pushed the patch-1 branch from e0bd09e to 258074d Compare April 5, 2019 16:06

36degrees reviewed Apr 24, 2019

View reviewed changes

edwardhorsford and others added 4 commits April 24, 2019 14:43

Don't output an empty ul if 0 items provided

beaa3b9

Add summary list dummy column where some rows have no actions

da0a6a3

Fix summary list column widths with missing actions

f91d932

Allow custom classes on summary list rows

1d3e6ea

Support no-border modifer on rows

0bc8906

Also fixes the 1px row height jump when borders are removed

colinrotherham force-pushed the patch-1 branch from 258074d to 0bc8906 Compare April 24, 2019 15:57

Update CHANGELOG

e3268eb

36degrees approved these changes Apr 24, 2019

View reviewed changes

36degrees added this to the Next milestone Apr 25, 2019

aliuk2012 approved these changes Apr 25, 2019

View reviewed changes

aliuk2012 merged commit 161ec33 into alphagov:master Apr 25, 2019

aliuk2012 mentioned this pull request Apr 25, 2019

Release v2.11.0 #1296

Merged

aliuk2012 mentioned this pull request Apr 25, 2019

Bump govuk-frontend from v2.10.0 to v2.11.0 alphagov/govuk-prototype-kit#741

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't output an empty ul if 0 items provided #1259

Don't output an empty ul if 0 items provided #1259

edwardhorsford commented Mar 28, 2019

timpaul commented Apr 3, 2019

colinrotherham commented Apr 5, 2019 •

edited

Loading

36degrees commented Apr 24, 2019

colinrotherham commented Apr 24, 2019

36degrees commented Apr 24, 2019

36degrees Apr 24, 2019

colinrotherham Apr 24, 2019

36degrees Apr 24, 2019

colinrotherham Apr 24, 2019

36degrees Apr 24, 2019

colinrotherham Apr 24, 2019

36degrees Apr 24, 2019

36degrees Apr 24, 2019

colinrotherham commented Apr 24, 2019 •

edited

Loading

36degrees commented Apr 24, 2019

colinrotherham commented Apr 24, 2019

36degrees commented Apr 24, 2019

colinrotherham commented Apr 24, 2019

36degrees left a comment

aliuk2012 left a comment

	{% set hasActions = false %}
	{% set anyRowHasActions = false %}

Don't output an empty ul if 0 items provided #1259

Don't output an empty ul if 0 items provided #1259

Conversation

edwardhorsford commented Mar 28, 2019

timpaul commented Apr 3, 2019

colinrotherham commented Apr 5, 2019 • edited Loading

36degrees commented Apr 24, 2019

colinrotherham commented Apr 24, 2019

36degrees commented Apr 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colinrotherham commented Apr 24, 2019 • edited Loading

36degrees commented Apr 24, 2019

colinrotherham commented Apr 24, 2019

36degrees commented Apr 24, 2019

colinrotherham commented Apr 24, 2019

36degrees left a comment

Choose a reason for hiding this comment

aliuk2012 left a comment

Choose a reason for hiding this comment

colinrotherham commented Apr 5, 2019 •

edited

Loading

colinrotherham commented Apr 24, 2019 •

edited

Loading