Check component item arrays are not empty before outputting markup

[nataliecarey]

Overview

When using the Nunjucks components there are a number of optional arrays, there's one behaviour when you provide something and another when you provide nothing. At HMRC we've found some confusion over the difference between providing nothing and providing an empty array.

This change standardises that behaviour so that an empty array is treated as nothing being provided.

Scope

This only affects the Nunjucks interface in its handling of empty arrays, users who aren't providing empty arrays will not be affected by this change.

There are no CSS or JS changes

Tests

We have added tests for the new behaviour.

[silasl]

This PR addresses the issue raised here #1618

[NickColley]

Spoke to Nat and they've said that they're going to be away for a week but @silasl (hi 👋 ) can help out in that time.

We'll review this as a team tomorrow in our weekly triage session.

[36degrees]

It'd be good to remove the change to package-lock.json, which I believe is unrelated and just down to different versions of npm being used.

(I've not had a chance to review the rest of the PR yet)

[36degrees]

Thanks @natcarey!

[NickColley]

This looks great to me! Thanks a lot.

I tried to help out and add a CHANGELOG entry myself but I don't have permission to push to your branch.

- [Pull request #1638: Check component item arrays are not empty before outputting markup](https://github.com/alphagov/govuk-frontend/pull/1638).

I think if we update the CHANGELOG and get another approval from someone else on the team we can get this merged and released in v3.4.0

[hannalaakso]

This is a useful change, thanks for all your work @natcarey.

Please note that we shouldn't be committing any package files in this PR. They will get generated as part of the release process. So those need to be removed before this can be merged.

[NickColley]

We had a small review internally, two things I missed:

1. some changes have been made in the package directory instead of the src directory
2. we are wondering if there are more places we need to do this: for instance accordion, breadcrumbs, select also have an array of items

Oops sorry for the double feedback 😇

[NickColley]

We're going to release 3.4.0 then do a small patch release with this once you've had time to look at this :)

[nataliecarey]

I've updated with that feedback and I'm working as usual this week.

Thanks all.

[nataliecarey]

I just spotted this comment "we are wondering if there are more places we need to do this: for instance accordion, breadcrumbs, select also have an array of items", I haven't addressed that yet. I'll look at that this morning.

[nataliecarey]

@NickColley said:

we are wondering if there are more places we need to do this: for instance accordion, breadcrumbs, select also have an array of items

I don't think these are affected by the same problem.

These examples are going to behave strangely when an empty array or no array is provided but the behaviour of an empty array matches the behaviour of no array. In the components I've updated there is a difference between the handling of "nothing" and an empty array.

Adding conditions around the template would help avoid a nonsense output but it isn't needed for consistency between these two behaviours. For example govukAccordion has a for loop which iterates the items and checks each one is not empty; If there's an empty array or no array the outer div (.govuk-accordion) is still output but it will be empty. A condition of {% if params.items | length %} would help with this but it changes the behaviour for both an empty array and no array.

[NickColley]

Something I missed, this changelog entry needs to be in an 'unreleased section' instead of being in the previous release

[nataliecarey]

Morning. Just to check I've got this right - I'm putting it on Line 6 so the start of the file reads:

# Changelog

## Unreleased

### New features
- [Pull request #1638: Check component item arrays are not empty before outputting markup](https://github.com/alphagov/govuk-frontend/pull/1638).

Is that right?

[NickColley]

Yeah that looks great but 'New features' should be 'Fixes'

[36degrees]

Sorry, this probably needs updating again as v3.4.0 has now been released.

[hannalaakso]

This is looking great 🙌 Just need @NickColley to give a second team thumbs up.

I don't think these are affected by the same problem. These examples are going to behave strangely when an empty array or no array is provided but the behaviour of an empty array matches the behaviour of no array. In the components I've updated there is a difference between the handling of "nothing" and an empty array.

I think @natcarey is correct here. The other components that utilise the items array are accordion, checkboxes, radios and select. With these ones there is no redundant markup output if items is empty, like there is with the ones fixed in this PR. I guess we could decide to make it so that whenever you use arrays in templates you should check the length, regardless of whether anything is output if the array is empty but I think that's separate from this work as the scope of this PR was to "Check component item arrays are not empty before outputting markup".

[NickColley]

Great work thanks Nat! Also thanks @hannalaakso for the sharp eye on the review 😅