Standardising null and empty arrays #77
Conversation
src/components/header/template.njk
Outdated
| {{ params.serviceName }} | ||
| </a> | ||
| {% endif %} | ||
| {% if params.navigation %} |
There was a problem hiding this comment.
Isn't the idea of this change to not show the navigation if an empty list is provided?
There was a problem hiding this comment.
Yeah, I would have thought the same. So should be:
| {% if params.navigation %} | |
| {% if params.navigation | length %} |
There was a problem hiding this comment.
Hmm, I got this from GDS. It's a decision between consistency and correctness I guess. I'm happy to add it.
There was a problem hiding this comment.
Is this not what the PR is for though? Just asking because I might be misinterpreting the reason for the PR. Should this be upstreamed if this is accepted? I think the navigation might have been changed slightly recently so may not be applicable anymore.
src/components/header/template.njk
Outdated
| @@ -56,38 +55,29 @@ | |||
| {% endif %} | |||
| </a> | |||
| </div> | |||
|
|
|||
| {% if params.serviceName or params.navigation or params.signOutHref or params.languageToggle %} | |||
| {% if params.serviceName or params.navigation %} | |||
There was a problem hiding this comment.
Isn't the params.signOutHref and params.languageToggle on of the reasons this was forked initially? Should we keep these in?
There was a problem hiding this comment.
+1 What's the reason for taking these out?
src/components/header/template.njk
Outdated
| {{ params.serviceName }} | ||
| </a> | ||
| {% endif %} | ||
| {% if params.navigation %} |
There was a problem hiding this comment.
Yeah, I would have thought the same. So should be:
| {% if params.navigation %} | |
| {% if params.navigation | length %} |
src/components/header/template.njk
Outdated
| @@ -56,38 +55,29 @@ | |||
| {% endif %} | |||
| </a> | |||
| </div> | |||
|
|
|||
| {% if params.serviceName or params.navigation or params.signOutHref or params.languageToggle %} | |||
| {% if params.serviceName or params.navigation | length %} | |||
There was a problem hiding this comment.
Do we still need the params.signOutHref here?
There was a problem hiding this comment.
Yes we do, I'll add that back in. Sorry.
| </nav> | ||
| {% endif %} | ||
| {% if params.serviceName %} | ||
| <a href="{{ params.serviceUrl }}" class="govuk-header__link govuk-header__link--service-name"> |
There was a problem hiding this comment.
There's a discussion going on at the moment about if the service name is displayed but serviceUrl isn't supplied. Not a change for this PR necessarily but wanted to mention.
There was a problem hiding this comment.
Yeah, it's a good conversation to have but I'd see it as separate to this PR.
|
Updated :) |
|
Closing as superceded by #81 |
matthewmascord
deleted the
PLATUI-345_standardising-null-and-empty-arrays
branch
This is the HMRC equivalent of alphagov/govuk-frontend#1638 which is best explained in this issue alphagov/govuk-frontend#1618.
This PR includes an update of the
headercomponent which had fallen out of sync withgovuk-frontend. No other changes were required in the production code but I've added a test to make sure the output remains the same in these cases whereitemListis used inadd-to-a-list.