855 update the top nav for check and submit#888
Conversation
…task-list-to-results-page
…der to imports, sets, extends
WalkthroughThis pull request applies a series of updates across multiple view templates, test suites, configuration files, and filters. The changes primarily introduce a dynamic navigation system using a new variable Changes
Sequence Diagram(s)sequenceDiagram
participant U as User Request
participant T as Template Engine
participant G as getNavigationLinks Filter
participant N as Navigation Links Repository
U->>T: Request view rendering
T->>G: Call getNavigationLinks(currentUrl, linkKeys)
G->>N: Retrieve predefined navigation links
N-->>G: Return link details
G-->>T: Return navigationItems array
T-->>U: Render view with dynamic navigation
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/views/organisations/issueTable.html (1)
141-141:⚠️ Potential issueRemove stray character.
There's a stray 'p' character that could cause template rendering issues.
- } %}p + } %}
🧹 Nitpick comments (4)
src/views/organisations/dataview.html (1)
6-17: Consider simplifying the active state logic.The current implementation can be made more concise by removing redundant checks:
- active: currentPath and currentPath.startsWith('/organisations') + active: currentPath?.startsWith('/organisations')src/views/organisations/datasetTaskList.html (1)
5-16: Consider extracting navigation items to a shared template.The navigation structure is duplicated across multiple templates. Consider moving it to a shared include file to improve maintainability.
Example implementation:
{# _shared-navigation.html #} {% set navigationItems = [ { href: "/organisations", text: "Find your organisation", active: currentPath?.startsWith('/organisations') }, { href: "/guidance", text: "Guidance", active: currentPath?.startsWith('/guidance') } ]%}Then in each template:
{% include "_shared-navigation.html" %}src/views/organisations/issueDetails.html (1)
10-10: Consider using a more specific path check.The current check
startsWith('/organisations')might incorrectly set the active state for unrelated paths that happen to start with '/organisations/'. Consider using a more precise check.- active: currentPath and currentPath.startsWith('/organisations') + active: currentPath and (currentPath === '/organisations' || currentPath.startsWith('/organisations/'))src/views/organisations/dataset-overview.html (1)
53-53: Consider moving URL styling to CSS.The URL styling is currently defined inline. Consider moving it to a CSS class for better maintainability.
-{% set urlStyle = 'text-overflow: ellipsis; overflow: hidden; white-space: nowrap; width: 400px; display: block;' %} +{% set urlClass = 'app-url-truncate' %}Add this to your CSS file:
.app-url-truncate { text-overflow: ellipsis; overflow: hidden; white-space: nowrap; width: 400px; display: block; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/views/check/results/results.html(1 hunks)src/views/components/app-header.html(1 hunks)src/views/organisations/dataset-overview.html(2 hunks)src/views/organisations/datasetTaskList.html(1 hunks)src/views/organisations/dataview.html(1 hunks)src/views/organisations/find.html(1 hunks)src/views/organisations/get-started.html(1 hunks)src/views/organisations/http-error.html(1 hunks)src/views/organisations/issueDetails.html(1 hunks)src/views/organisations/issueTable.html(1 hunks)src/views/organisations/overview.html(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/views/check/results/results.html
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests / test
🔇 Additional comments (8)
src/views/components/app-header.html (1)
9-14: Well-structured implementation of configurable navigation!The change from hardcoded navigation to a dynamic
navigationItemsvariable with a sensible default makes the header component more flexible and reusable.src/views/organisations/find.html (1)
3-14: Same navigation structure as other templates.This implements the same navigation pattern discussed in previous files.
src/views/organisations/http-error.html (1)
4-15: Consistent implementation of navigation structure.The navigation implementation maintains consistency with other templates, which is excellent for user experience.
src/views/organisations/issueDetails.html (1)
6-17: LGTM! Navigation implementation follows the standard pattern.The navigation items are correctly structured with proper active state logic.
src/views/organisations/issueTable.html (1)
8-19: LGTM! Navigation implementation is consistent.The navigation structure matches the standard pattern used across other templates.
src/views/organisations/overview.html (1)
6-17: LGTM! Navigation implementation aligns with the standard pattern.The navigation structure and active state logic are consistent with other templates.
src/views/organisations/dataset-overview.html (1)
27-38: LGTM! Navigation implementation follows the standard pattern.The navigation structure and active state logic are consistent with other templates.
src/views/organisations/get-started.html (1)
3-14: LGTM! Navigation implementation matches the standard pattern.The navigation structure and active state logic are consistent with other templates.
GeorgeGoodall-GovUk
had a problem deploying
to
development
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/views/landing.html (1)
6-17: Add null check for currentPath variable.The currentPath checks could be more defensive. Consider using the optional chaining operator to handle cases where currentPath might be undefined.
- active: currentPath and currentPath.startsWith('/organisations') + active: currentPath?.startsWith('/organisations') ?? falseconfig/default.yaml (1)
20-21: Plan to remove the deprecatedserviceNameproperty.The comment indicates that
serviceNameis deprecated in favour ofserviceNames. Consider creating a follow-up task to remove this property.src/views/cookies.html (1)
8-19: LGTM! Navigation implementation is consistent with other templates.The
navigationItemsarray follows the established pattern, maintaining consistency across the application.Consider extracting these common navigation items into a shared template or macro to follow the DRY principle. This would make future updates easier and reduce the risk of inconsistencies.
+{# shared/navigation.njk #} +{% macro commonNavigation(currentPath) %} + {% set navigationItems = [ + { + href: "/organisations", + text: "Find your organisation", + active: currentPath and currentPath.startsWith('/organisations') + }, + { + href: "/guidance", + text: "Guidance", + active: currentPath and currentPath.startsWith('/guidance') + } + ] %} + {{ navigationItems }} +{% endmacro %}Then in each template:
-{% set navigationItems = [ - { - href: "/organisations", - text: "Find your organisation", - active: currentPath and currentPath.startsWith('/organisations') - }, - { - href: "/guidance", - text: "Guidance", - active: currentPath and currentPath.startsWith('/guidance') - } -]%} +{% from "shared/navigation.njk" import commonNavigation %} +{% set navigationItems = commonNavigation(currentPath) %}src/views/error.html (3)
1-16: Consider moving template extension before variable declarationsFor better template inheritance and clarity, it's typically recommended to place the
extendsstatement at the beginning of the template, before any variable declarations or imports.+{% extends "layouts/main.html" %} {% from "govuk/components/button/macro.njk" import govukButton %} {% set pageName = "Error" %} {% set navigationItems = [ ... ]%} -{% extends "layouts/main.html" %}
18-23: Consider enhancing error presentationThe current error display could be improved to be more user-friendly and follow GDS error pattern guidelines. Consider adding:
- A clear error summary
- More descriptive error messages for common scenarios
- Stack traces only in non-production environments
- <h1 class="govuk-heading-xl">Error</h1> + <h1 class="govuk-heading-xl">Sorry, there is a problem with the service</h1> - <h2>{{err.code + " - " if err.code}}{{err.message}}</h2> + <p class="govuk-body">{{err.message}}</p> + <p class="govuk-body">Try again later.</p> {%if err.stack %} + {% if environment !== "production" %} <pre>{{err.stack | safe}}</pre> + {% endif %} {% endif %}
25-28: Consider adding error tracking IDFor better support and debugging, consider adding a unique error reference ID that users can quote when reporting issues.
{{ govukButton({ text: "Start again", href: "/" }) }} + <p class="govuk-body-s"> + If you need to contact support, please quote error reference: {{errorId}} + </p>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
config/default.yaml(1 hunks)src/views/accessibility.html(1 hunks)src/views/cookies.html(1 hunks)src/views/error.html(1 hunks)src/views/landing.html(1 hunks)src/views/privacy-notice.html(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests / test
🔇 Additional comments (5)
src/views/landing.html (1)
6-17: Well-structured navigation configuration.The navigation implementation:
- Follows GDS patterns
- Makes navigation configurable per page as intended
- Clearly separates concerns between structure and presentation
config/default.yaml (1)
24-25: LGTM! Service names are now more concise and specific.The updated service names better reflect their distinct purposes, improving clarity for users.
src/views/accessibility.html (1)
4-15: LGTM! Navigation implementation is clean and consistent.The
navigationItemsarray provides a clear structure for navigation links with proper active state handling.src/views/privacy-notice.html (2)
151-151: Verify the privacy notice update date.The privacy notice shows a future update date of 21 June 2024. Please confirm if this is intentional or if it should reflect a current or past date.
4-15: Verify thecurrentPathvariable availability.The navigation structure looks good and aligns with the configurable navigation objectives. However, please ensure that the
currentPathvariable is properly defined and passed to this template.❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Search for currentPath variable definition rg -A 5 "currentPath.*=" src/Length of output: 1362
Confirm
currentPathProvisioningOur investigation shows that the
currentPathvariable is not defined within thesrc/views/privacy-notice.htmlfile itself. However, similar usage in files likesrc/views/components/sub-navigation.htmlsuggests thatcurrentPathis expected to be provided through the global or contextual parameters (e.g. by the controller or layout).Please ensure that the context passed to this and related templates reliably includes
currentPath.
GeorgeGoodall-GovUk
had a problem deploying
to
development
GitHub Actions
Failure
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
test/unit/javascript/components/cookie-banner.test.js (1)
60-69:⚠️ Potential issueRemove duplicate test case.
This test case is a duplicate of the one at lines 27-36. Both verify the same component properties. Consider removing this redundant test to maintain a cleaner test suite.
src/views/submit/check-answers.html (1)
60-60: 🛠️ Refactor suggestionStandardise URL prefixes across change links.
The change links use inconsistent URL prefixes:
/submit/lpa-details/submit/dataset-details/submit-endpoint/dataset-detailsThis inconsistency could lead to confusion and maintenance issues.
Standardise all URLs to use the same prefix, preferably
/submit/:- href: "/submit-endpoint/dataset-details", + href: "/submit/dataset-details",Also applies to: 94-94, 111-111, 128-128
🧹 Nitpick comments (2)
test/unit/views/components/cookie-banner.test.js (1)
8-8: Standardise service type casing.The service type is set to lowercase 'manage' here but uppercase 'Manage' in the JavaScript test file. Consider standardising the casing across all files.
test/unit/javascript/components/cookie-banner.test.js (1)
10-10: Standardise service type casing.The service type is set to uppercase 'Manage' here but lowercase 'manage' in the view test file. Consider standardising the casing across all files.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/views/cookies.html(1 hunks)src/views/submit/check-answers.html(1 hunks)test/unit/check/confirmationPage.test.js(1 hunks)test/unit/endpointSubmissionForm/confirmationPage.test.js(1 hunks)test/unit/findPage.test.js(1 hunks)test/unit/javascript/components/cookie-banner.test.js(1 hunks)test/unit/views/components/cookie-banner.test.js(1 hunks)test/unit/views/submit/check-answers.test.js(1 hunks)test/unit/views/submit/choose-datasetPage.test.js(1 hunks)test/unit/views/submit/dataset-details.test.js(1 hunks)test/unit/views/submit/lpa-detailsPage.test.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/unit/views/submit/dataset-details.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/views/cookies.html
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests / test
🔇 Additional comments (8)
test/unit/views/components/cookie-banner.test.js (1)
8-8: Verify heading text matches service type.The cookie banner heading text mentions "Submit" despite the service type being set to "manage". Please verify if this is the intended behaviour.
Also applies to: 18-18
test/unit/check/confirmationPage.test.js (1)
19-19: LGTM! Title update aligns with service context changes.The removal of "and update" from the page title is consistent with the broader changes being made across the service.
test/unit/endpointSubmissionForm/confirmationPage.test.js (1)
19-19: LGTM! Title update maintains consistency.The change aligns with the updated service context and maintains consistency with other confirmation pages.
test/unit/views/submit/choose-datasetPage.test.js (1)
15-15: LGTM! Title update follows service-wide changes.The page title update maintains consistency with the service's new context whilst preserving all test functionality.
test/unit/views/submit/lpa-detailsPage.test.js (1)
15-15: LGTM! Title update completes service-wide changes.The page title update aligns with the service's new context. Let's verify the consistency of these changes across the codebase.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify that all page titles have been updated consistently # Search for any remaining instances of "Submit and update your planning data" rg -l "Submit and update your planning data" # Search for the new format to confirm widespread adoption rg -l "Submit your planning data"Length of output: 843
The submission page title update is consistent with other submission pages.
The new title "Submit your planning data" appears across all submission-related tests (including lpa-detailsPage.test.js), confirming the update. The old string showing "Submit and update your planning data" remains in non-submission areas, which appears intentional.
test/unit/findPage.test.js (1)
22-22: LGTM! Service name update is consistent.The change to use
config.serviceNames.managealigns with the standardisation effort across the application.test/unit/views/submit/check-answers.test.js (1)
24-24: LGTM! Service name simplification is consistent.The simplified page title aligns with the standardisation effort across the application.
src/views/submit/check-answers.html (1)
8-8: LGTM! Service type update better reflects page purpose.The change from 'Check' to 'Submit' more accurately represents the page's functionality.
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
GeorgeGoodall-GovUk
had a problem deploying
to
development
GitHub Actions
Failure
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/filters/filters.js(2 hunks)src/filters/getNavigationLinks.js(1 hunks)src/views/accessibility.html(1 hunks)src/views/cookies.html(1 hunks)src/views/error.html(1 hunks)src/views/includes/navigation.html(1 hunks)src/views/landing.html(1 hunks)src/views/organisations/dataset-overview.html(2 hunks)src/views/organisations/datasetTaskList.html(1 hunks)src/views/organisations/dataview.html(1 hunks)src/views/organisations/find.html(1 hunks)src/views/organisations/get-started.html(1 hunks)src/views/organisations/http-error.html(1 hunks)src/views/organisations/issueDetails.html(1 hunks)src/views/organisations/issueTable.html(1 hunks)src/views/organisations/overview.html(1 hunks)src/views/privacy-notice.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- src/views/error.html
- src/views/accessibility.html
- src/views/cookies.html
- src/views/organisations/issueDetails.html
- src/views/organisations/dataview.html
- src/views/organisations/overview.html
- src/views/privacy-notice.html
- src/views/organisations/http-error.html
- src/views/organisations/datasetTaskList.html
- src/views/organisations/dataset-overview.html
- src/views/organisations/issueTable.html
- src/views/organisations/get-started.html
- src/views/organisations/find.html
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests / test
🔇 Additional comments (2)
src/filters/getNavigationLinks.js (1)
19-34: LGTM! Well-structured navigation link generation.The function is well-implemented with proper error handling and filtering of undefined links. The JSDoc documentation clearly describes the parameters and return type.
src/filters/filters.js (1)
14-14: LGTM! Clean integration of navigation links filter.The import and filter registration are properly implemented, following the existing patterns in the file.
Also applies to: 52-52
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
This reverts commit de7cf1c.
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/unit/filters/getNavigationLinks.test.js (1)
5-72: LGTM! Comprehensive test coverage for main scenarios.The test cases are well-structured and cover the primary use cases effectively.
Consider these improvements:
- Extract common test data to reduce duplication:
const DEFAULT_LINKS = ['organisations', 'guidance'] const DEFAULT_NAVIGATION = [ { href: '/organisations', text: 'Find your organisation', active: false }, { href: '/guidance', text: 'Guidance', active: false } ]
- Add test cases for edge cases:
it('handles URLs with query parameters', () => { const result = getNavigationLinks('/guidance?param=value', ['organisations', 'guidance']) expect(result[1].active).toBe(true) }) it('handles URLs with trailing slashes', () => { const result = getNavigationLinks('/guidance/', ['organisations', 'guidance']) expect(result[1].active).toBe(true) }) it('handles malformed currentUrl parameter', () => { expect(() => getNavigationLinks(null, ['organisations'])).not.toThrow() expect(() => getNavigationLinks(undefined, ['organisations'])).not.toThrow() })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/filters/getNavigationLinks.js(1 hunks)src/views/accessibility.html(1 hunks)src/views/cookies.html(1 hunks)src/views/error.html(1 hunks)src/views/landing.html(1 hunks)src/views/organisations/dataset-overview.html(2 hunks)src/views/organisations/datasetTaskList.html(1 hunks)src/views/organisations/dataview.html(1 hunks)src/views/organisations/find.html(1 hunks)src/views/organisations/get-started.html(1 hunks)src/views/organisations/http-error.html(1 hunks)src/views/organisations/issueDetails.html(1 hunks)src/views/organisations/issueTable.html(1 hunks)src/views/organisations/overview.html(1 hunks)src/views/privacy-notice.html(1 hunks)test/unit/filters/getNavigationLinks.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- src/views/cookies.html
- src/views/landing.html
- src/views/organisations/issueDetails.html
- src/views/organisations/http-error.html
- src/views/error.html
- src/views/privacy-notice.html
- src/views/organisations/find.html
- src/views/organisations/datasetTaskList.html
- src/views/organisations/dataview.html
- src/views/organisations/issueTable.html
- src/views/organisations/overview.html
- src/views/organisations/get-started.html
- src/views/accessibility.html
- src/views/organisations/dataset-overview.html
- src/filters/getNavigationLinks.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests / test
🔇 Additional comments (1)
test/unit/filters/getNavigationLinks.test.js (1)
1-4: LGTM! Well-structured test setup.The imports and test suite setup follow best practices with clear, focused imports and a descriptive test suite name.
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
5 participants
Preview Link
https://submit-pr-888.herokuapp.com/
What type of PR is this? (check all applicable)
Description
Removes nav links from check and submit
Also:
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Before
After
Added/updated tests?
QA sign off
Summary by CodeRabbit
Summary by CodeRabbit