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

Jump to bottom

Apg 556 tech UI 1.5 and 5.1 page titles could be improved #823

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Apg 556 tech UI 1.5 and 5.1 page titles could be improved #823

wants to merge 3 commits into from

Conversation

Giruvagen
Copy link

Context

Updates the way we build titles from copying the page heading to using one of 3 formats:

  • Page title
  • Page title with suffix (For now always Accredited Programmes)
  • Either of the above with dynamic content (i.e status filter for referrals)

Release process documentation

As part of our continuous deployment strategy we must ensure that this work is
ready to be released once merged.

Pre-merge

  • There are changes required to the Accredited Programmes API for this change to work...
    • ... and they have been released to production already

Post-merge

  • This adds, extends or meaningfully modifies a feature...
  • This makes new expectations of the API...
    • ... and I have notified the API developer(s) of changes to the contract tests (Pact), or the API is already compliant
  • Manually approve release to preprod
  • Manually approve release to prod

jsrobertson
jsrobertson requested changes Dec 16, 2024
View reviewed changes
Copy link
Contributor

@jsrobertson jsrobertson left a comment

Choose a reason for hiding this comment

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

I would be keen to see if we can just append - Accredited programmes - DPS to all page titles by default. Would drop the need to have pageTitleSuffix specified in the majority of cases.

In general, we don’t do squash commits so we try to keep the commit history clean of merge commits by rebasing against main. So, ideally each commit should pass build/tests etc and have a specific purpose and commit message.

.node-version Outdated Show resolved Hide resolved
integration_tests/pages/refer/new/additionalInformation.ts Outdated Show resolved Hide resolved
integration_tests/pages/shared/withdraw/category.ts Outdated Show resolved Hide resolved
integration_tests/pages/shared/withdraw/reason.ts Outdated Show resolved Hide resolved
server/controllers/shared/reasonController.test.ts Outdated Show resolved Hide resolved
server/controllers/shared/reasonController.test.ts Outdated Show resolved Hide resolved
server/controllers/shared/risksAndNeedsController.ts Outdated Show resolved Hide resolved
jsrobertson
jsrobertson requested changes Dec 18, 2024
View reviewed changes
Copy link
Contributor

@jsrobertson jsrobertson left a comment

Choose a reason for hiding this comment

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

Few small tweaks but should be good afterwards. 👍

Could we do some rebasing to tidy the commits up a bit too, please?

server/views/dashboard/index.njk
@@ -2,7 +2,7 @@

{% extends "../partials/layout.njk" %}

{% set customPageTitleEnd = "Home" %}
{% set pageTitleOverride = "Accredited Programmes service" %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Think the controller might need hideTitleServiceName.

server/controllers/shared/updateStatusSelectionController.ts Outdated Show resolved Hide resolved
server/controllers/shared/updateStatusSelectionController.test.ts Outdated Show resolved Hide resolved
server/controllers/shared/risksAndNeedsController.test.ts Outdated
@@ -174,6 +174,8 @@ describe('RisksAndNeedsController', () => {
...sharedPageData,
alcoholMisuseSummaryListRows,
hasData: true,
hideTitleServiceName: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

All the instances of hideTitleServiceName and pageTitleOverride in this file could get added to sharedPageData on line 122 to avoid the repetition.

server/controllers/shared/referralsController.ts Outdated Show resolved Hide resolved
server/controllers/assess/updateStatusDecisionController.test.ts Outdated Show resolved Hide resolved
integration_tests/pages/shared/withdraw/category.ts Outdated
@@ -4,7 +4,9 @@ import type { ReferralStatusCategory, ReferralStatusReason } from '@accredited-p

export default class WithdrawCategoryPage extends Page {
constructor() {
super('Withdraw referral')
super('Withdraw referral', {
pageTitleOverride: '',
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed.

integration_tests/pages/refer/new/complete.ts Outdated Show resolved Hide resolved
integration_tests/pages/notFound.ts Outdated Show resolved Hide resolved
integration_tests/pages/authError.ts Outdated Show resolved Hide resolved
@Giruvagen Giruvagen force-pushed the APG-556-TECH-UI-1.5-and-5.1-Page-titles-could-be-improved branch from 810f1ae to aa8ffae Compare December 18, 2024 15:26
jsrobertson
jsrobertson requested changes Dec 19, 2024
View reviewed changes
server/controllers/shared/risksAndNeedsController.test.ts
Copy link
Contributor

Choose a reason for hiding this comment

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

All the instances of ,

pageTitleOverride: `Risks and needs for referral to ${coursePresenter.displayName}`

can also be put in sharedPageData. Would also be nice to clean up any of the empty lines that have been added from moving hideTitleServiceName there.

@Giruvagen Giruvagen force-pushed the APG-556-TECH-UI-1.5-and-5.1-Page-titles-could-be-improved branch from 05e2fdb to c0ef4dd Compare December 19, 2024 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsrobertson jsrobertson jsrobertson requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Giruvagen @jsrobertson