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

Do not make the service name in the header a link if no serviceUrl is provided #2617

Merged
merged 2 commits into from
May 23, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
16 changes: 16 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,22 @@

## Unreleased

### Recommended changes

#### Replace deprecated `govuk-header__link--service-name` class in the header

If you're not using the Nunjucks macros, replace any instances in the header of the class `govuk-header__link--service-name` with `govuk-header__service-name`.

We've deprecated the `govuk-header__link--service-name` class, and will remove it in a future major release.

This change was introduced in [pull request #2617: Do not make the service name in the header a link if no `serviceUrl` is provided](https://github.com/alphagov/govuk-frontend/pull/2617).

### Fixes

We’ve made fixes to GOV.UK Frontend in the following pull requests:

- [#2617: Do not make the service name in the header a link if no `serviceUrl` is provided](https://github.com/alphagov/govuk-frontend/pull/2617)

## 4.1.0 (Feature release)

### New features
Expand Down
2 changes: 1 addition & 1 deletion app/app.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ describe(`http://localhost:${PORT}`, () => {
requestPath(templatePath, (err, res) => {
const $ = cheerio.load(res.body)
const $header = $('.govuk-header')
const $serviceName = $header.find('.govuk-header__link--service-name')
const $serviceName = $header.find('.govuk-header__service-name')
expect($serviceName.html()).toContain('Nom du service')
done(err)
})
Expand Down
3 changes: 3 additions & 0 deletions src/govuk/components/header/_index.scss
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,9 @@
}
}

// The govuk-header__link--service-name class is deprecated - use
// govuk-header__service-name instead.
.govuk-header__service-name,
Comment on lines +145 to +147
Copy link
Contributor Author

@36degrees 36degrees May 13, 2022

Choose a reason for hiding this comment

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

Are we happy with this approach? If so, will need to create an issue to remove the govuk-header__link--service-name class and attach it to the milestone for v5.0, and include this in the release notes as a deprecation.

The alternative I think is to either:

  • live with having a link modifier class on something that isn't a link (which feels icky)
  • keep both classes around forever

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've created an issue to remove the deprecated class here: #2636

.govuk-header__link--service-name {
display: inline-block;
margin-bottom: govuk-spacing(2);
Expand Down
4 changes: 4 additions & 0 deletions src/govuk/components/header/header.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,10 @@ examples:
serviceName: Service Name
serviceUrl: '/components/header'

- name: with service name but no service url
data:
serviceName: Service Name

- name: with navigation
data:
navigation:
Expand Down
12 changes: 9 additions & 3 deletions src/govuk/components/header/template.njk
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,15 @@
{% if params.serviceName or params.navigation %}
<div class="govuk-header__content">
{% if params.serviceName %}
<a href="{{ params.serviceUrl }}" class="govuk-header__link govuk-header__link--service-name">
{{ params.serviceName }}
</a>
{% if params.serviceUrl %}
<a href="{{ params.serviceUrl }}" class="govuk-header__link govuk-header__service-name">
{{ params.serviceName }}
</a>
{% else%}
<span class="govuk-header__service-name">
{{ params.serviceName }}
</span>
{% endif %}
{% endif %}
{% if params.navigation %}
<nav aria-label="{{ params.navigationLabel | default('Menu') }}" class="govuk-header__navigation {{ params.navigationClasses if params.navigationClasses }}">
Expand Down
16 changes: 13 additions & 3 deletions src/govuk/components/header/template.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,17 +84,27 @@ describe('header', () => {
const $ = render('header', examples['with service name'])

const $component = $('.govuk-header')
const $serviceName = $component.find('.govuk-header__link--service-name')
const $serviceName = $component.find('.govuk-header__service-name')
expect($serviceName.text().trim()).toEqual('Service Name')
})

it('renders with service url', () => {
it('wraps the service name with a link when a url is provided', () => {
const $ = render('header', examples['with service name'])

const $component = $('.govuk-header')
const $serviceName = $component.find('.govuk-header__link--service-name')
const $serviceName = $component.find('.govuk-header__service-name')
expect($serviceName.get(0).tagName).toEqual('a')
expect($serviceName.attr('href')).toEqual('/components/header')
})

it('does not use a link when no service url is provided', () => {
const $ = render('header', examples['with service name but no service url'])

const $component = $('.govuk-header')
const $serviceName = $component.find('.govuk-header__service-name')
expect($serviceName.get(0).tagName).toEqual('span')
expect($serviceName.attr('href')).toBeUndefined()
})
})

describe('with navigation', () => {
Expand Down