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.

Sign up for GitHub

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

Standardising null and empty arrays #77

Closed
wants to merge 5 commits into from
Closed

Standardising null and empty arrays #77

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a35d7fa
Upgraded header, using the latest version fron GDS.
nataliecarey Jun 4, 2020
34ec18e
Adding test to prove that the output is the same for add-to-a-list
nataliecarey Jun 5, 2020
88e9113
Fixed consistency on navigatoin - should be up-streamed to GDS.
nataliecarey Jun 8, 2020
c7b1038
Resetting the port after accidentally changing it.
nataliecarey Jun 8, 2020
220fa14
Re-introduced conditoins for HMRC-specific sections.
nataliecarey Jun 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion config/paths.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,6 @@
"packageTest": "__tests__/packageTest",
"ports": {
"app": 3000,
"test": 8888
"test": 8887
timsb marked this conversation as resolved.
Show resolved Hide resolved
}
}
26 changes: 13 additions & 13 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 15 additions & 0 deletions src/components/add-to-a-list/template.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,4 +171,19 @@ describe('Add to a list', () => {
expect($form.attr('action')).toBe('#addItem')
})
})

describe('Empty, null and missing itemLists', () => {
function overrideItemList (itemList) {
const params = {...examples.default}
params.itemList = itemList
return params
}

it('should have the same output for all versions of no items being provided', () => {
const outputs = [undefined, null, []].map(itemList => render('add-to-a-list', overrideItemList(itemList)).html())

expect(outputs[0]).toEqual(outputs[1])
timsb marked this conversation as resolved.
Show resolved Hide resolved
expect(outputs[0]).toEqual(outputs[2])
})
})
})
53 changes: 37 additions & 16 deletions src/components/header/header.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,11 @@ params:
- name: productName
type: string
required: false
description: Header title that is placed next to GOV.UK. Used for product names (i.e. Pay, Verify)
description: Product name, used when the product name follows on directly from ‘GOV.UK’. For example, GOV.UK Pay or GOV.UK Design System. In most circumstances, you should use serviceName.
- name: serviceName
type: string
required: false
description: Header title that is placed next to GOV.UK. Used for product names (i.e. Pay, Verify)
description: The name of your service, included in the header.
- name: serviceUrl
type: string
required: false
Expand All @@ -27,7 +27,11 @@ params:
- name: text
type: string
required: false
description: Text of the navigation item.
description: Text for the navigation item. If `html` is provided, the `text` argument will be ignored.
- name: html
type: string
required: false
description: HTML for the navigation item. If `html` is provided, the `text` argument will be ignored.
- name: href
type: string
required: false
Expand Down Expand Up @@ -84,17 +88,16 @@ accessibilityCriteria: |
examples:
- name: default
description: The standard header as used on information pages on GOV.UK
language: en
data:
{}

- name: with-service-name
- name: with service name
description: If your service is more than a few pages long, you can help users understand where they are by adding the service name.
data:
serviceName: Service Name
serviceUrl: '/components/header'

- name: with-navigation
- name: with navigation
data:
navigation:
- href: '#1'
Expand All @@ -107,7 +110,7 @@ examples:
- href: '#4'
text: Navigation item 4

- name: with-service-name-and-navigation
- name: with service name and navigation
description: If you need to include basic navigation, contact or account management links.
data:
serviceName: Service Name
Expand All @@ -123,7 +126,7 @@ examples:
- href: '#4'
text: Navigation item 4

- name: with-large-navigation
- name: with large navigation
description: An edge case example with a large number of navitation items with long names used to test wrapping
data:
navigation:
Expand Down Expand Up @@ -160,13 +163,18 @@ examples:
- href: '/browse/working'
text: Working, jobs and pensions

- name: full-width
- name: with product name
data:
navigationClasses: govuk-header__navigation--end
productName: Product Name

- name: full width
data:
containerClasses: govuk-header__container--full-width
navigationClasses: govuk-header__navigation--end
productName: Product Name

- name: full-width-with-navigation
- name: full width with navigation
data:
containerClasses: govuk-header__container--full-width
navigationClasses: govuk-header__navigation--end
Expand All @@ -180,19 +188,32 @@ examples:
- href: '#3'
text: Navigation item 3

- name: sign-out-link
- name: navigation item with html
data:
serviceName: Service Name
serviceUrl: '/components/header'
navigation:
- href: '#1'
html: <em>Navigation item 1</em>
active: true
- href: '#2'
html: <em>Navigation item 2</em>
- href: '#3'
html: <em>Navigation item 3</em>

- name: sign out link
data:
serviceName: Example Service
signOutHref: /sign-out

- name: sign-out-link-in-welsh
- name: sign out link in welsh
description: This puts the signout link into the header and sets the whole header to be welsh. At the time of writing the language only affects the signout text.
data:
serviceName: Gwasanaeth enghreifftiol
language: cy
signOutHref: /sign-out

- name: with-language-toggle-english
- name: with language toggle english
data:
serviceName: Example Service
language: en
Expand All @@ -202,7 +223,7 @@ examples:
cy:
href: "/components/header/with-language-toggle-welsh/preview"

- name: with-language-toggle-welsh
- name: with language toggle welsh
data:
serviceName: Gwasanaeth enghreifftiol
language: cy
Expand All @@ -212,7 +233,7 @@ examples:
cy:
href: "/components/header/with-language-toggle-welsh/preview"

- name: with-hmrc-banner-english
- name: with hmrc banner english
data:
serviceName: Service with HMRC Banner
language: en
Expand All @@ -223,7 +244,7 @@ examples:
cy:
href: "/components/header/with-hmrc-banner-welsh/preview"

- name: with-hmrc-banner-welsh
- name: with hmrc banner welsh
data:
serviceName: Gwasanaeth gyda Banner CThEM
language: cy
Expand Down
68 changes: 29 additions & 39 deletions src/components/header/template.njk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@
{% from "../banner/macro.njk" import hmrcBanner %}

<header class="govuk-header hmrc-header {{ params.classes if params.classes }} {{ 'hmrc-header--with-additional-navigation' if params.signOutHref or params.languageToggle }}" role="banner" data-module="header"
{%- for attribute, value in params.attributes %} {{ attribute }}="{{ value }}"{% endfor %}>
{%- for attribute, value in params.attributes %} {{attribute}}="{{value}}"{% endfor %}>
<div class="govuk-header__container {{ params.containerClasses | default('govuk-width-container') }}">
<div class="govuk-header__logo">
<a href="{{ params.homepageUrl | default('/') }}" class="govuk-header__link govuk-header__link--homepage">
<span class="govuk-header__logotype">
{# We use an inline SVG for the crown so that we can cascade the
{#- We use an inline SVG for the crown so that we can cascade the
currentColor into the crown whilst continuing to support older browsers
which do not support external SVGs without a Javascript polyfill. This
adds approximately 1kb to every page load.
Expand All @@ -20,19 +20,19 @@
treat it as an interactive element - without this it will be
'focusable' when using the keyboard to navigate. #}
<svg
role="presentation"
aria-hidden="true"
focusable="false"
class="govuk-header__logotype-crown"
xmlns="http://www.w3.org/2000/svg"
viewbox="0 0 132 97"
height="32"
height="30"
width="36"
>
<path
fill="currentColor" fill-rule="evenodd"
d="M25 30.2c3.5 1.5 7.7-.2 9.1-3.7 1.5-3.6-.2-7.8-3.9-9.2-3.6-1.4-7.6.3-9.1 3.9-1.4 3.5.3 7.5 3.9 9zM9 39.5c3.6 1.5 7.8-.2 9.2-3.7 1.5-3.6-.2-7.8-3.9-9.1-3.6-1.5-7.6.2-9.1 3.8-1.4 3.5.3 7.5 3.8 9zM4.4 57.2c3.5 1.5 7.7-.2 9.1-3.8 1.5-3.6-.2-7.7-3.9-9.1-3.5-1.5-7.6.3-9.1 3.8-1.4 3.5.3 7.6 3.9 9.1zm38.3-21.4c3.5 1.5 7.7-.2 9.1-3.8 1.5-3.6-.2-7.7-3.9-9.1-3.6-1.5-7.6.3-9.1 3.8-1.3 3.6.4 7.7 3.9 9.1zm64.4-5.6c-3.6 1.5-7.8-.2-9.1-3.7-1.5-3.6.2-7.8 3.8-9.2 3.6-1.4 7.7.3 9.2 3.9 1.3 3.5-.4 7.5-3.9 9zm15.9 9.3c-3.6 1.5-7.7-.2-9.1-3.7-1.5-3.6.2-7.8 3.7-9.1 3.6-1.5 7.7.2 9.2 3.8 1.5 3.5-.3 7.5-3.8 9zm4.7 17.7c-3.6 1.5-7.8-.2-9.2-3.8-1.5-3.6.2-7.7 3.9-9.1 3.6-1.5 7.7.3 9.2 3.8 1.3 3.5-.4 7.6-3.9 9.1zM89.3 35.8c-3.6 1.5-7.8-.2-9.2-3.8-1.4-3.6.2-7.7 3.9-9.1 3.6-1.5 7.7.3 9.2 3.8 1.4 3.6-.3 7.7-3.9 9.1zM69.7 17.7l8.9 4.7V9.3l-8.9 2.8c-.2-.3-.5-.6-.9-.9L72.4 0H59.6l3.5 11.2c-.3.3-.6.5-.9.9l-8.8-2.8v13.1l8.8-4.7c.3.3.6.7.9.9l-5 15.4v.1c-.2.8-.4 1.6-.4 2.4 0 4.1 3.1 7.5 7 8.1h.2c.3 0 .7.1 1 .1.4 0 .7 0 1-.1h.2c4-.6 7.1-4.1 7.1-8.1 0-.8-.1-1.7-.4-2.4V34l-5.1-15.4c.4-.2.7-.6 1-.9zM66 92.8c16.9 0 32.8 1.1 47.1 3.2 4-16.9 8.9-26.7 14-33.5l-9.6-3.4c1 4.9 1.1 7.2 0 10.2-1.5-1.4-3-4.3-4.2-8.7L108.6 76c2.8-2 5-3.2 7.5-3.3-4.4 9.4-10 11.9-13.6 11.2-4.3-.8-6.3-4.6-5.6-7.9 1-4.7 5.7-5.9 8-.5 4.3-8.7-3-11.4-7.6-8.8 7.1-7.2 7.9-13.5 2.1-21.1-8 6.1-8.1 12.3-4.5 20.8-4.7-5.4-12.1-2.5-9.5 6.2 3.4-5.2 7.9-2 7.2 3.1-.6 4.3-6.4 7.8-13.5 7.2-10.3-.9-10.9-8-11.2-13.8 2.5-.5 7.1 1.8 11 7.3L80.2 60c-4.1 4.4-8 5.3-12.3 5.4 1.4-4.4 8-11.6 8-11.6H55.5s6.4 7.2 7.9 11.6c-4.2-.1-8-1-12.3-5.4l1.4 16.4c3.9-5.5 8.5-7.7 10.9-7.3-.3 5.8-.9 12.8-11.1 13.8-7.2.6-12.9-2.9-13.5-7.2-.7-5 3.8-8.3 7.1-3.1 2.7-8.7-4.6-11.6-9.4-6.2 3.7-8.5 3.6-14.7-4.6-20.8-5.8 7.6-5 13.9 2.2 21.1-4.7-2.6-11.9.1-7.7 8.8 2.3-5.5 7.1-4.2 8.1.5.7 3.3-1.3 7.1-5.7 7.9-3.5.7-9-1.8-13.5-11.2 2.5.1 4.7 1.3 7.5 3.3l-4.7-15.4c-1.2 4.4-2.7 7.2-4.3 8.7-1.1-3-.9-5.3 0-10.2l-9.5 3.4c5 6.9 9.9 16.7 14 33.5 14.8-2.1 30.8-3.2 47.7-3.2z"
></path>
{# Fallback PNG image for older browsers.
{#- Fallback PNG image for older browsers.

The <image> element is a valid SVG element. In SVG, you would specify
the URL of the image file with the xlink:href – as we don't reference an
Expand All @@ -42,8 +42,7 @@

In other browsers <image> is synonymous for the <img> tag and will be
interpreted as such, displaying the fallback image. #}
<image src="{{ params.assetsPath | default('/assets/images') }}/govuk-logotype-crown.png"
class="govuk-header__logotype-crown-fallback-image"></image>
<image src="{{ params.assetsPath | default('/assets/images') }}/govuk-logotype-crown.png" xlink:href="" class="govuk-header__logotype-crown-fallback-image" width="36" height="32"></image>
</svg>
<span class="govuk-header__logotype-text">
GOV.UK
Expand All @@ -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 %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't the params.signOutHref and params.languageToggle on of the reasons this was forked initially? Should we keep these in?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 What's the reason for taking these out?

<div class="govuk-header__content">
{% if params.serviceName %}
<a href="{{ params.serviceUrl }}" class="govuk-header__link govuk-header__link--service-name">
{{ params.serviceName }}
</a>
{% endif %}

{% if params.navigation %}
<button type="button" role="button" class="govuk-header__menu-button js-header-toggle"
aria-controls="navigation"
aria-label="Show or hide Top Level Navigation">Menu
</button>
<nav>
<ul id="navigation"
class="govuk-header__navigation {{ params.navigationClasses if params.navigationClasses }}"
aria-label="Top Level Navigation">
{% for item in params.navigation %}
{% if item.href and item.text %}
<li class="govuk-header__navigation-item{{ ' govuk-header__navigation-item--active' if item.active }}">
<a class="govuk-header__link"
href="{{ item.href }}"{% for attribute, value in item.attributes %} {{ attribute }}="{{ value }}"{% endfor %}
>
{{ item.text }}
</a>
</li>
{% endif %}
{% endfor %}
</ul>
</nav>
{% endif %}
{% if params.serviceName %}
<a href="{{ params.serviceUrl }}" class="govuk-header__link govuk-header__link--service-name">
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's a good conversation to have but I'd see it as separate to this PR.

{{ params.serviceName }}
</a>
{% endif %}
{% if params.navigation %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the idea of this change to not show the navigation if an empty list is provided?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I would have thought the same. So should be:

Suggested change
{% if params.navigation %}
{% if params.navigation | length %}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I got this from GDS. It's a decision between consistency and correctness I guess. I'm happy to add it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

<button type="button" class="govuk-header__menu-button govuk-js-header-toggle" aria-controls="navigation" aria-label="Show or hide Top Level Navigation">Menu</button>
<nav>
<ul id="navigation" class="govuk-header__navigation {{ params.navigationClasses if params.navigationClasses }}" aria-label="Top Level Navigation">
{% for item in params.navigation %}
{% if item.href and (item.text or item.html) %}
<li class="govuk-header__navigation-item{{ ' govuk-header__navigation-item--active' if item.active }}">
<a class="govuk-header__link" href="{{ item.href }}"{% for attribute, value in item.attributes %} {{attribute}}="{{value}}"{% endfor %}>
{{ item.html | safe if item.html else item.text }}
</a>
</li>
{% endif %}
{% endfor %}
</ul>
</nav>
{% endif %}

{% if params.signOutHref %}
<nav class="hmrc-sign-out-nav">
Expand All @@ -110,7 +100,7 @@
</nav>
{% endif %}
</div>
{% endif %}
{% endif %}
</div>
</header>
<div class="govuk-width-container">
Expand Down
Loading