Skip to content
This repository has been archived by the owner on Nov 29, 2023. It is now read-only.

Feature/title tag #56

Merged
merged 9 commits into from
May 25, 2021
Merged

Feature/title tag #56

merged 9 commits into from
May 25, 2021

Conversation

NickFitz
Copy link
Contributor

Adds <title>s for each page.

Draws on https://www.gov.uk/service-manual/design/writing-for-user-interfaces#style which has some pointers, and additional discussion at alphagov/govuk-design-system-backlog#157

A couple more fixtures were added, which meant some of the existing Cypress tests had to be tweaked. In order to avoid having to change them again if further fixtures are added in future, I've replaced tests using contains() with invoke('text').matches() using regular expressions with \d+ instead of the embedded numerical values that had changed.

@NickFitz NickFitz marked this pull request as draft May 24, 2021 10:38
@NickFitz
Copy link
Contributor Author

Just realised there's a conflicting change with main so changing this to draft status for now.

@NickFitz NickFitz marked this pull request as ready for review May 24, 2021 11:16
Copy link
Contributor

@mforner13 mforner13 left a comment

Choose a reason for hiding this comment

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

Just a few suggestions :)

{% extends "base.html" %}

{% block page_title %}Current monthly update - {{ strategic_action.name }} update – {{ supply_chain.name }} – {{ block.super }}{% endblock page_title %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Think you could get rid of the 'update' after the strategic action name since it says it in the first bit :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, that's left over from the form pages where the update is being created, so not really relevant here :)

Comment on lines 49 to 56
context('The <title> of', () => {

describe('The Home Page', () => {
it('has the correct title', () => {
cy.visit(urls.home);
cy.title().should('equal', expectedTitles.home());
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

With the tests being called 'has the correct title' I don't think you'd need the context line. Alternatively you could have 'The title of - The {page} - is correct'? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The context are mainly not necessary, I think, so I've tidied it up a bit :)

const strategicActionUpdate = strategicActionUpdates.find((strategicActionUpdate) => strategicActionUpdate.fields.strategic_action === strategicAction.pk && strategicActionUpdate.fields.slug === '05-2021');


const urls = {
Copy link
Contributor

Choose a reason for hiding this comment

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

It could be really useful to move this out to be accessible by other specs :) Could be in support/utils?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good idea :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, it's a bit more complicated than that… as it needs the various fixtures passed to it, I'll make it callable.

Copy link
Contributor

@mforner13 mforner13 left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@NickFitz NickFitz requested a review from dev-digital May 24, 2021 16:14

const urls = urlBuilder(supplyChain, strategicAction, strategicActionUpdate);

const expectedTitles = {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Nice way to collate and organise all title tests.

{% extends "base.html" %}

{% block page_title %}Current monthly update - {{ strategic_action.name }} – {{ supply_chain.name }} – {{ block.super }}{% endblock page_title %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any recommendation/restrictions for the length of the title from GDS ?

Would this title show up as ?

Current monthly update - Strategic action 01 - Ceramics - Update supply chain information

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, there's very little advice - just the things I linked in the PR. HMRC recommend no more than four dash-separated components, so although it's a little verbose, it's within that limit.

# Conflicts:
#	defend_data_capture/supply_chains/templates/base.html
@NickFitz NickFitz merged commit bfdad45 into main May 25, 2021
@NickFitz NickFitz deleted the feature/title-tag branch May 25, 2021 11:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants