Feature/892 - Adopt MOJ Frontend mojFilter component - #920
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change migrates six bespoke filter panels to the MOJ Frontend ChangesMOJ filter migration
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (9)
apps/web/src/assets/js/filter-toggle.test.ts (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the module-level constant.
Rename
filterToggleButtonSpytoFILTER_TOGGLE_BUTTON_SPYand update its references. This follows the required naming rule for constants.As per coding guidelines, use
SCREAMING_SNAKE_CASEfor constants.Source: Coding guidelines
apps/web/src/pages/(list-types)/sjp-press-list/sjp-press-list.njk.test.ts (1)
187-197: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicated start-hidden assertions.
The "Filter toggle" block and the "Filter panel" block assert the same
data-start-hiddenvalues for the default case and forshowFilter: true. Keep one set. This reduces maintenance when the attribute contract changes.Also applies to: 223-236
apps/web/src/pages/(list-types)/sjp-public-list/sjp-public-list.njk (1)
76-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared SJP filter options markup.
Lines 76-128 duplicate
apps/web/src/pages/(list-types)/sjp-press-list/sjp-press-list.njklines 87-139 almost exactly: the hiddenartefactIdinput, the search input, and the postcode and prosecutor checkbox groups. Move the markup into a shared Nunjucks macro and call it from both templates. This keeps the two SJP filter panels in step.As per coding guidelines: "Prefer simple functional code, immutable data with
const, minimal duplication".Source: Coding guidelines
apps/web/src/pages/(list-types)/sjp-public-list/sjp-public-list.njk.test.ts (2)
531-541: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMerge the duplicated Welsh data-attribute assertions.
Line 534 and line 554 both assert
data-show-textequalscy.common.showFilters. Keep the assertion in one test.Also applies to: 551-557
502-508: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the script-token assertion with the press-list test.
This test checks for
"filter-toggle". The equivalent press-list test checks for"filterToggle". Use the same tokens in both tests so the guard covers the same strings.apps/web/src/pages/(public)/courts-tribunals-list/index.njk (1)
22-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGive the sub-jurisdiction group a real label.
The sub-jurisdiction checkboxes sit inside the jurisdiction fieldset. A
spanwith heading classes provides their only label. Screen readers do not associate thatspanwith the group. Use a nestedfieldsetwith alegend, or passfieldset: { legend: { text: item.subJurisdictionLabel } }togovukCheckboxes.apps/web/src/pages/(verified)/location-name-search/index.njk (1)
26-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared jurisdiction filter markup.
Lines 26-61 duplicate
apps/web/src/pages/(public)/courts-tribunals-list/index.njklines 12-47 exactly. Move the markup into a shared Nunjucks macro and call it from both templates.As per coding guidelines: "Prefer simple functional code, immutable data with
const, minimal duplication".Source: Coding guidelines
e2e-tests/tests/courts-tribunals-list.spec.ts (2)
383-389: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFold the structural check into the filter journey test.
This test only asserts that the panel and its inputs render. It replaced a behaviour test. The E2E guideline requires one test per complete user journey and forbids separate tests for individual checks. Move these assertions into the existing filter journey test.
As per coding guidelines: "Use Playwright E2E tests with one test per complete user journey... Do not create separate E2E tests for individual validations, translations, or accessibility checks".
Source: Coding guidelines
303-303: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer a role selector for the filter tags.
The tests locate tags by the
.moj-filter__tagclass. The tag is a link with a visually hidden "Remove this filter" label. Usepage.getByRole("link", { name: /Remove this filter/ })so the test breaks when the accessible name is lost, and so it does not depend on MOJ class names.As per coding guidelines: "use role or label selectors before test IDs".
Also applies to: 353-368, 448-448
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: db3554f0-6412-4183-b245-c920ce80cdfe
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (45)
apps/web/package.jsonapps/web/src/assets/css/dashboard.scssapps/web/src/assets/css/filter-overrides.scssapps/web/src/assets/css/filter-panel.scssapps/web/src/assets/css/list-types/sjp-filters.scssapps/web/src/assets/css/user-management.scssapps/web/src/assets/css/verified-pages.scssapps/web/src/assets/css/web.scssapps/web/src/assets/js/filter-panel.test.tsapps/web/src/assets/js/filter-panel.tsapps/web/src/assets/js/filter-toggle.test.tsapps/web/src/assets/js/filter-toggle.tsapps/web/src/assets/js/web.test.tsapps/web/src/assets/js/web.tsapps/web/src/pages/(list-types)/sjp-press-list/sjp-press-list.njkapps/web/src/pages/(list-types)/sjp-press-list/sjp-press-list.njk.test.tsapps/web/src/pages/(list-types)/sjp-public-list/sjp-public-list.njkapps/web/src/pages/(list-types)/sjp-public-list/sjp-public-list.njk.test.tsapps/web/src/pages/(public)/courts-tribunals-list/index.njkapps/web/src/pages/(public)/courts-tribunals-list/index.njk.test.tsapps/web/src/pages/(system-admin)/audit-log-list/cy.tsapps/web/src/pages/(system-admin)/audit-log-list/en.tsapps/web/src/pages/(system-admin)/audit-log-list/index.njkapps/web/src/pages/(system-admin)/audit-log-list/index.tsapps/web/src/pages/(system-admin)/find-users/cy.tsapps/web/src/pages/(system-admin)/find-users/en.tsapps/web/src/pages/(system-admin)/find-users/index.njkapps/web/src/pages/(system-admin)/find-users/index.njk.test.tsapps/web/src/pages/(system-admin)/jurisdiction-data-list/cy.tsapps/web/src/pages/(system-admin)/jurisdiction-data-list/en.tsapps/web/src/pages/(system-admin)/jurisdiction-data-list/index.njkapps/web/src/pages/(system-admin)/jurisdiction-data-list/index.njk.test.tsapps/web/src/pages/(verified)/location-name-search/index.njkapps/web/vite.build.tsdocs/tickets/892/plan.mddocs/tickets/892/review.mddocs/tickets/892/tasks.mddocs/tickets/892/ticket.mde2e-tests/tests/courts-tribunals-list.spec.tse2e-tests/tests/sjp-press-list.spec.tse2e-tests/tests/system-admin/user-management.spec.tse2e-tests/tests/verified-user/sjp-public-list.spec.tslibs/public-pages/src/flat-file/flat-file-service.tslibs/test-support/src/nunjucks-test-helper.tslibs/web-core/src/middleware/govuk-frontend/configure-govuk.ts
💤 Files with no reviewable changes (2)
- apps/web/src/assets/css/user-management.scss
- libs/public-pages/src/flat-file/flat-file-service.ts
| // object. Only these two content helpers remain: they style cells inside the | ||
| // results table, not the filter panel. | ||
| .audit-log-layout__word-break { | ||
| word-break: break-word; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace the deprecated word-break value.
Line 47 fails Stylelint because break-word is deprecated for word-break. Use overflow-wrap: anywhere to retain long-value wrapping.
Proposed fix
.audit-log-layout__word-break {
- word-break: break-word;
+ overflow-wrap: anywhere;
}🧰 Tools
🪛 Stylelint (17.14.0)
[error] 47-47: Deprecated keyword "break-word" for property "word-break" (declaration-property-value-keyword-no-deprecated)
(declaration-property-value-keyword-no-deprecated)
Source: Linters/SAST tools
| // Custom overrides on top of the MOJ Frontend filter component + filter-layout | ||
| // object (adopted in #892). These bring the migrated panels back towards the | ||
| // bespoke look the pages had before the migration, in the same spirit as | ||
| // hmcts/pip-frontend's main.scss filter overrides. Loaded from web.scss AFTER | ||
| // the MOJ @use lines and govuk index so these rules win on source order. | ||
| // |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Resolve the Stylelint errors.
Line 6 is an empty comment. Line 101 needs an empty line before the comment. Remove the empty comment and add the required separation.
Also applies to: 90-105
🧰 Tools
🪛 Stylelint (17.14.0)
[error] 6-6: Unexpected empty comment (scss/comment-no-empty)
(scss/comment-no-empty)
Source: Linters/SAST tools
| // The filter toggle (FilterToggleButton) shows/hides the panel by toggling | ||
| // `.moj-js-hidden`, which lives in MOJ's hidden utility — without this @use the | ||
| // class has no rule and the panel can never hide (breaks the SJP show/hide). | ||
| @use "@ministryofjustice/frontend/moj/utilities/hidden"; | ||
| @use "govuk-frontend/dist/govuk/index"; | ||
| // The MOJ vendor layer above configures the shared govuk "base" module with | ||
| // $govuk-include-default-font-face: false, which suppresses GOV.UK's GDS Transport | ||
| // @font-face for the whole stylesheet (govuk helpers/_typography.scss gates on that flag). | ||
| // The gds-transport mixin is re-included below (after all @use rules) to re-emit them. | ||
| @use "govuk-frontend/dist/govuk/helpers/font-faces--internal" as govuk-font-faces; | ||
| @use "./filter-overrides.scss"; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Fix the Stylelint errors before merge.
Line 9 and Line 14 require an empty line before the comments. Line 19 must omit the .scss extension from the Sass @use path.
Proposed fix
`@use` "`@ministryofjustice/frontend/moj/objects/filter-layout`";
+
// The filter toggle (FilterToggleButton) shows/hides the panel by toggling
@@
`@use` "govuk-frontend/dist/govuk/index";
+
// The MOJ vendor layer above configures the shared govuk "base" module with
@@
-@use "./filter-overrides.scss";
+@use "./filter-overrides";🧰 Tools
🪛 Stylelint (17.14.0)
[error] 9-9: Expected empty line before comment (scss/double-slash-comment-empty-line-before)
(scss/double-slash-comment-empty-line-before)
[error] 14-14: Expected empty line before comment (scss/double-slash-comment-empty-line-before)
(scss/double-slash-comment-empty-line-before)
[error] 19-19: Unexpected extension ".scss" in @use (scss/load-partial-extension)
(scss/load-partial-extension)
Source: Linters/SAST tools
| {% set selectedCategories = [] %} | ||
| {% if filters.postcodes and filters.postcodes.length > 0 %} | ||
| {% set postcodeTags = [] %} | ||
| {% for postcode in filters.postcodes %} | ||
| {% set removeHref = "?artefactId=" + list.artefactId %} | ||
| {% for pc in filters.postcodes %}{% if pc != postcode %}{% set removeHref = removeHref + "&postcode=" + pc %}{% endif %}{% endfor %} | ||
| {% if filters.prosecutors %}{% for p in filters.prosecutors %}{% set removeHref = removeHref + "&prosecutor=" + p %}{% endfor %}{% endif %} | ||
| {% set postcodeTags = (postcodeTags.push({ text: postcode, href: removeHref }), postcodeTags) %} | ||
| {% endfor %} | ||
| {% set selectedCategories = (selectedCategories.push({ heading: { text: postcodeFilterHeading }, items: postcodeTags }), selectedCategories) %} | ||
| {% endif %} | ||
| {% if filters.prosecutors and filters.prosecutors.length > 0 %} | ||
| {% set prosecutorTags = [] %} | ||
| {% for prosecutor in filters.prosecutors %} | ||
| {% set removeHref = "?artefactId=" + list.artefactId %} | ||
| {% if filters.postcodes %}{% for pc in filters.postcodes %}{% set removeHref = removeHref + "&postcode=" + pc %}{% endfor %}{% endif %} | ||
| {% for p in filters.prosecutors %}{% if p != prosecutor %}{% set removeHref = removeHref + "&prosecutor=" + p %}{% endif %}{% endfor %} | ||
| {% set prosecutorTags = (prosecutorTags.push({ text: prosecutor, href: removeHref }), prosecutorTags) %} | ||
| {% endfor %} | ||
| {% set selectedCategories = (selectedCategories.push({ heading: { text: prosecutorFilterHeading }, items: prosecutorTags }), selectedCategories) %} | ||
| {% endif %} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Unencoded filter values in the MOJ selected-filter removal URLs. Both SJP templates build removeHref by concatenating raw postcode and prosecutor values into a query string. A value that contains &, + or # breaks the query string and applies the wrong filter set after removal.
apps/web/src/pages/(list-types)/sjp-press-list/sjp-press-list.njk#L141-L161: apply| urlencodeto eachpcandpvalue in bothremoveHrefloops.apps/web/src/pages/(list-types)/sjp-public-list/sjp-public-list.njk#L130-L150: apply| urlencodeto eachpcandpvalue in bothremoveHrefloops.
📍 Affects 2 files
apps/web/src/pages/(list-types)/sjp-press-list/sjp-press-list.njk#L141-L161(this comment)apps/web/src/pages/(list-types)/sjp-public-list/sjp-public-list.njk#L130-L150
| it("should render filter tags with an accessible remove link", () => { | ||
| const { $ } = renderList({ filters: { postcodes: ["SW1"], prosecutors: [] } }); | ||
|
|
||
| const removeLink = $('a[aria-label="Remove SW1 filter"]'); | ||
| expect(removeLink).toHaveLength(1); | ||
| expect(removeLink.hasClass("filter-tag-remove")).toBe(true); | ||
| const tag = $(".moj-filter__tag"); | ||
| expect(tag).toHaveLength(1); | ||
| expect(tag.text()).toContain("SW1"); | ||
| expect(tag.find(".govuk-visually-hidden").text()).toContain("Remove this filter"); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
MOJ Frontend 10 mojFilter selected filters tag "Remove this filter" hidden text override
💡 Result:
In the MOJ Frontend Filter component, the text "Remove this filter" is implemented as hidden text for screen readers within each filter tag [1][2]. To override or customize this text, you typically modify the HTML template in your application where the moj-filter-tags are generated. The standard implementation uses a span element with the govuk-visually-hidden class inside the anchor tag [1][2]: Remove this filter Filter name Because the MOJ Design System documentation provides this as a template pattern rather than a rigid, immutable component property, you have direct control over the markup in your Nunjucks template [1][2]. You can change the content inside the govuk-visually-hidden span to whatever text is appropriate for your service's context [1]. If you are using the Nunjucks macro to render the component, note that the component structure is designed to be flexible; you can often override the rendering of the selected filters section by providing your own HTML partials or modifying the data passed to the filter component [1][3]. If the component does not natively support a specific override for this string via a macro parameter, updating the template directly is the standard approach within the Ministry of Justice design patterns [1][2].
Citations:
- 1: https://design-patterns.service.justice.gov.uk/components/filter/
- 2: https://design-patterns.service.justice.gov.uk/patterns/filter-a-list/
- 3: Filter ministryofjustice/moj-frontend#197
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- candidate files ---'
git ls-files | rg 'sjp-press-list|filter|moj-frontend|govuk-frontend' | head -120
printf '%s\n' '--- test context ---'
sed -n '250,315p' 'apps/web/src/pages/(list-types)/sjp-press-list/sjp-press-list.njk.test.ts'
printf '%s\n' '--- page references ---'
rg -n --glob '*.njk' --glob '*.ts' --glob '*.json' 'moj-filter|Remove this filter|filterTag|filter tag|filters' apps/web/src apps/web/package.json package.jsonRepository: hmcts/cath-service
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -e
PAGE='apps/web/src/pages/(list-types)/sjp-press-list/sjp-press-list.njk'
TEST='apps/web/src/pages/(list-types)/sjp-press-list/sjp-press-list.njk.test.ts'
printf '%s\n' '--- template filter markup ---'
rg -n -C 8 'moj-filter__tag|govuk-visually-hidden|selectedFilters|clearFilters|showFilters|hideFilters|filter' "$PAGE"
printf '%s\n' '--- locale and render setup ---'
sed -n '1,125p' "$TEST"
printf '%s\n' '--- Welsh tests ---'
rg -n -C 8 'Welsh|Welsh|lng=cy|cy\.|locale|language' "$TEST"
printf '%s\n' '--- dependency declarations ---'
rg -n -C 3 'moj-frontend|govuk-frontend|nunjucks' apps/web/package.json package.json pnpm-lock.yaml yarn.lock package-lock.json 2>/dev/null || trueRepository: hmcts/cath-service
Length of output: 36388
🏁 Script executed:
#!/bin/bash
set -e
PAGE='apps/web/src/pages/(list-types)/sjp-press-list/sjp-press-list.njk'
TEST='apps/web/src/pages/(list-types)/sjp-press-list/sjp-press-list.njk.test.ts'
printf '%s\n' '--- template filter markup ---'
rg -n -C 8 'moj-filter__tag|govuk-visually-hidden|selectedFilters|clearFilters|showFilters|hideFilters|filter' "$PAGE"
printf '%s\n' '--- locale and render setup ---'
sed -n '1,125p' "$TEST"
printf '%s\n' '--- Welsh tests ---'
rg -n -C 8 'Welsh|lng=cy|cy\.|locale|language' "$TEST"
printf '%s\n' '--- dependency declarations ---'
rg -n -C 3 'moj-frontend|govuk-frontend|nunjucks' apps/web/package.json package.json pnpm-lock.yaml yarn.lock package-lock.json 2>/dev/null || trueRepository: hmcts/cath-service
Length of output: 36388
🌐 Web query:
github ministryofjustice frontend v10.0.1 moj components filter macro.njk "Remove this filter"
💡 Result:
In the Ministry of Justice (MOJ) Frontend library, the Filter component is used via the mojFilter Nunjucks macro [1]. If you are looking to remove an individual filter or selected filter item from the interface, this is typically handled by adjusting the configuration passed to the macro, specifically within the selectedFilters object [1]. The selectedFilters parameter in the mojFilter macro allows you to define categories and items that are currently active [1]. To remove a filter, you should: 1. Locate the selectedFilters section in your Nunjucks template [1]. 2. Identify the categories array within that object [1]. 3. Find the specific items object corresponding to the filter you wish to remove [1]. 4. Remove that item entry from the array [1]. Each item in the selectedFilters configuration includes an href property, which represents the link to remove that specific filter (often by redirecting the user to a URL that excludes that filter parameter) [1]. Removing the item from your Nunjucks configuration will stop it from rendering in the "Selected filters" section of the component [1]. If you are attempting to remove or hide the entire "Selected filters" section programmatically or via CSS, note that this section is a standard part of the component's structure [1]. For further customization or to address specific behavioral issues (such as modifying accessibility attributes like aria-haspopup), users often rely on custom JavaScript or CSS to override the component's default behavior, as the macro configuration itself is intended to support standard patterns [2][3]. For definitive guidance on the parameters available in your specific version (v10.0.1), you can inspect the Nunjucks macro options on the official MOJ Design System component page [1][4].
Citations:
- 1: https://design-patterns.service.justice.gov.uk/components/filter/
- 2: Filter a list ministryofjustice/moj-frontend#717
- 3: Filter ministryofjustice/moj-frontend#197
- 4: https://design-patterns.service.justice.gov.uk/components/
🏁 Script executed:
#!/bin/bash
set -e
BASE='https://raw.githubusercontent.com/ministryofjustice/moj-frontend/v10.0.1'
for FILE in \
'src/moj/components/filter/macro.njk' \
'src/moj/components/filter/_macro.njk' \
'src/moj/components/filter/template.njk' \
'src/moj/components/filter/filter.njk'
do
URL="$BASE/$FILE"
BODY=$(curl -fsSL "$URL" 2>/dev/null || true)
if [ -n "$BODY" ]; then
printf '%s\n' "--- $URL ---"
printf '%s\n' "$BODY" | rg -n -C 5 'Remove this filter|visually-hidden|selectedFilters|categories|items|remove' || printf '%s\n' "$BODY"
fi
done
printf '%s\n' '--- tagged tree candidates ---'
curl -fsSL 'https://api.github.com/repos/ministryofjustice/moj-frontend/git/trees/v10.0.1?recursive=1' |
jq -r '.tree[]?.path' |
rg 'filter|Filter' |
head -80Repository: hmcts/cath-service
Length of output: 1850
🏁 Script executed:
#!/bin/bash
python3 - <<'PY'
from pathlib import Path
from urllib.request import urlopen
page = Path("apps/web/src/pages/(list-types)/sjp-press-list/sjp-press-list.njk").read_text()
macro = urlopen(
"https://raw.githubusercontent.com/ministryofjustice/moj-frontend/v10.0.1/src/moj/components/filter/template.njk",
timeout=10,
).read().decode()
assert "postcodeTags.push({ text: postcode, href: removeHref })" in page
assert "prosecutorTags.push({ text: prosecutor, href: removeHref })" in page
assert '<span class="govuk-visually-hidden">Remove this filter</span>' in macro
assert "item.remove" not in macro
assert "item.removeText" not in macro
assert "item.visuallyHiddenText" not in macro
print("selected filter items provide only text and href")
print("MOJ Frontend 10.0.1 supplies a fixed English removal label")
print("no item-level removal-label override is supported")
PYRepository: hmcts/cath-service
Length of output: 2442
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
import subprocess
page = Path("apps/web/src/pages/(list-types)/sjp-press-list/sjp-press-list.njk").read_text()
macro = subprocess.check_output(
[
"curl", "-fsSL",
"https://raw.githubusercontent.com/ministryofjustice/moj-frontend/v10.0.1/src/moj/components/filter/template.njk",
],
text=True,
)
assert "postcodeTags = (postcodeTags.push({ text: postcode, href: removeHref }), postcodeTags)" in page
assert "prosecutorTags = (prosecutorTags.push({ text: prosecutor, href: removeHref }), prosecutorTags)" in page
assert '<span class="govuk-visually-hidden">Remove this filter</span>' in macro
assert "item.removeText" not in macro
assert "item.visuallyHiddenText" not in macro
print("selected filter items provide only text and href")
print("MOJ Frontend 10.0.1 supplies a fixed English removal label")
print("no item-level removal-label override is supported")
PYRepository: hmcts/cath-service
Length of output: 314
Localise the selected-filter removal label
mojFilter hard-codes Remove this filter, so Welsh selected-filter tags expose English screen-reader text. Render a localised removal label through an override or custom tag markup, and add a Welsh test with an active filter.
Source: Coding guidelines
| {% if selectedSubJurisdictions.length > 0 %} | ||
| {% set subJurisdictionTags = [] %} | ||
| {% for i in range(0, selectedSubJurisdictions.length) %} | ||
| {% set subJurisdictionTags = (subJurisdictionTags.push({ text: selectedSubJurisdictionsDisplay[i], href: subJurisdictionRemoveUrls[i] }), subJurisdictionTags) %} | ||
| {% endfor %} | ||
| {% set selectedCategories = (selectedCategories.push({ heading: { text: filterHeading }, items: subJurisdictionTags }), selectedCategories) %} | ||
| {% endif %} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Sub-jurisdiction tag group is labelled "Filter". Both templates set the sub-jurisdiction selected-filter category heading to filterHeading. The rendered group heading then reads "Filter" next to "Jurisdiction" and "Region".
apps/web/src/pages/(public)/courts-tribunals-list/index.njk#L57-L63: replacefilterHeadingwith a dedicated sub-jurisdiction heading fromen.tsandcy.ts.apps/web/src/pages/(verified)/location-name-search/index.njk#L71-L77: replacefilterHeadingwith the same dedicated sub-jurisdiction heading.
📍 Affects 2 files
apps/web/src/pages/(public)/courts-tribunals-list/index.njk#L57-L63(this comment)apps/web/src/pages/(verified)/location-name-search/index.njk#L71-L77
| <div class="govuk-form-group"> | ||
| <label class="govuk-label govuk-!-font-weight-bold">{{ actionsLabel }}</label> | ||
| <p class="govuk-body-s">No actions available yet</p> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Localise the empty-actions message.
Welsh users see No actions available yet when no actions exist. Add this message to both locale objects, pass it from index.ts, and render that value here.
Proposed change
- <p class="govuk-body-s">No actions available yet</p>
+ <p class="govuk-body-s">{{ noActionsAvailableText }}</p>As per coding guidelines, “Every page must provide English and Welsh content and select the current locale through the i18n middleware.”
Source: Coding guidelines
| ### JavaScript init (FilterToggleButton wiring, .js-enabled class) | ||
| In `web.ts`, after `initAll()`: | ||
| ```ts | ||
| import { FilterToggleButton } from "@ministryofjustice/frontend"; | ||
| // inside the DOMContentLoaded / else branches: | ||
| const filter = document.querySelector<HTMLElement>(".moj-filter"); | ||
| if (filter) { | ||
| new FilterToggleButton({ | ||
| bigModeMediaQuery: "(min-width: 48.0625em)", | ||
| startHidden: filter.dataset.startHidden === "true", | ||
| toggleButton: { showText: ..., hideText: ..., classes: "govuk-button--secondary" }, | ||
| closeButton: { text: ... }, | ||
| // toggleButtonContainer / closeButtonContainer selectors as required by v10 API | ||
| }); | ||
| } | ||
| ``` | ||
| - The `.js-enabled` body class is already present (GOV.UK `template.njk:35`) — no work. | ||
| - Show/hide/close button text must come from the page's locale data, passed via | ||
| `data-*` attributes on the container (NOT interpolated into JS — see §3), then read | ||
| in `web.ts`. Confirm the exact v10 `FilterToggleButton` option names against the | ||
| installed package before finalising (the ticket lists `toggleButtonContainer` / | ||
| `closeButtonContainer` / `bigModeMediaQuery` / `startHidden` / `toggleButton.showText` | ||
| / `hideText`). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files =="
git ls-files | rg '(^docs/tickets/892/plan\.md$|filter-toggle\.ts$|web\.ts$|package\.json$|package-lock\.json$|yarn\.lock$|pnpm-lock\.yaml$)' || true
echo "== docs excerpt =="
if [ -f docs/tickets/892/plan.md ]; then
nl -ba docs/tickets/892/plan.md | sed -n '100,150p;218,240p'
fi
echo "== package manifests =="
for f in package.json; do [ -f "$f" ] && nl -ba "$f" | sed -n '1,220p'; done
for f in pkg.json package.json; do [ -f "$f" ] && nl -ba "$f" | sed -n '1,220p'; done
echo "== search FilterToggleButton usages =="
rg -n "FilterToggleButton|initFilterToggle|toggle logic|web\.ts" .Repository: hmcts/cath-service
Length of output: 3509
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== docs excerpt =="
sed -n '110,145p;220,236p' docs/tickets/892/plan.md || true
echo "== filter-toggle.ts =="
sed -n '1,220p' apps/web/src/assets/js/filter-toggle.ts || true
echo "== web.ts relevant imports/usages =="
grep -n "import\\|FilterToggleButton\\|filter-toggle\\|initFilterToggle\\|DOMContentLoaded" apps/web/src/assets/js/web.ts || true
echo "== package pins for `@ministryofjustice/frontend` =="
python3 - <<'PY'
import json, pathlib
for p in pathlib.Path('.').rglob('package.json'):
try:
data=json.loads(p.read_text())
except Exception:
continue
deps={}
for key in ('dependencies','devDependencies','peerDependencies','optionalDependencies'):
deps.update(data.get(key,{}) or {})
if '`@ministryofjustice/frontend`' in deps:
print(p, deps['`@ministryofjustice/frontend`'])
PY
echo "== yarn lock frontend entries =="
grep -A30 '`@ministryofjustice/frontend`@' yarn.lock | head -80 || true
echo "== local package source/types if installed =="
find apps/web -path '*node_modules/@ministryofjustice/frontend*' -type f | head -50 || true
find . -path '*node_modules/@ministryofjustice/frontend*' -type f | head -50 || trueRepository: hmcts/cath-service
Length of output: 10301
Fix the FilterToggleButton wiring example.
The FilterToggleButton constructor takes (filter, options), and initFilterToggle() from filter-toggle.ts owns that construction. Update the plan so web.ts only calls initFilterToggle() after initAll(), and remove new FilterToggleButton(...) from this section.
🧰 Tools
🪛 LanguageTool
[style] ~136-~136: Would you like to use the Oxford spelling “finalizing”? The spelling ‘finalising’ is also correct.
Context: ... against the installed package before finalising (the ticket lists `toggleButtonContaine...
(OXFORD_SPELLING_Z_NOT_S)
| 2. **Two acceptance criteria unverifiable at runtime in this sandbox (AC8, AC9)** | ||
| - AC8 (axe passes on all six + keyboard reach to panel and close button) and AC9 (`yarn test:e2e` green) require a browser and Redis, neither available here. The E2E specs have been correctly updated to MOJ selectors (`.moj-filter`, `.moj-filter__tag`, tag-as-remove-link) and the code paths look right, but "axe passes" and "e2e green" are runtime claims that have not been executed. | ||
| - **Recommendation**: Run `yarn test:e2e` (and the axe checks embedded in the journey specs) in a CI/local environment with Redis + browsers before merge, and confirm green. Until then these two ACs stay `- [~]`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files 'docs/tickets/892/*' || true
echo "== review relevant =="
if [ -f docs/tickets/892/review.md ]; then
nl -ba docs/tickets/892/review.md | sed -n '1,140p'
fi
echo "== tasks relevant =="
if [ -f docs/tickets/892/tasks.md ]; then
nl -ba docs/tickets/892/tasks.md | sed -n '1,120p'
fi
echo "== references =="
rg -n "AC8|AC9|test:e2e|AxeBuilder|Next Steps|APPROVED|unverified|not runnable|green|passed" docs/tickets/892 || trueRepository: hmcts/cath-service
Length of output: 335
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== deterministic evidence status probe =="
python3 - <<'PY'
from pathlib import Path
import re
docs = {
"review": Path("docs/tickets/892/review.md"),
"tasks": Path("docs/tickets/892/tasks.md"),
}
for name, path in docs.items():
if not path.exists():
print(f"{name}: MISSING")
continue
text = path.read_text()
print(f"\n--- {name} ---")
for needle in [
"Two acceptance criteria un",
"Next Steps",
"Overall Assessment",
"APPROVED",
"Axe passes on all six",
"yarn test:e2e",
"3673 passed",
"not executable",
"not runnable",
"run green",
"8 passed",
"Tally:",
"verification",
]:
if needle in text:
print(f"FOUND {needle:!r}")
# count checkbox states where likely enough
for m in re.finditer(r'\[[\s*\-xX~o]\].*(Axe passes|yarn test:e2e)', text):
print("CHECKBOX", m.group(0)[:160].replace("\n", " "))
PYRepository: hmcts/cath-service
Length of output: 325
Align the runtime verification evidence across the ticket docs.
The records claim AC8/AC9 were unrunnable or unverified, then later record those checks as run green. If AC8 and AC9 passed in CI/local, remove the partial/unverified status consistently and cite the exact commands/results. If they did not run, keep them partial and do not mark the PR as approved on that evidence.
🧰 Tools
🪛 LanguageTool
[typographical] ~30-~30: It seems that a comma is missing after this introductory phrase.
Context: ...owsers before merge, and confirm green. Until then these two ACs stay - [~]. ## 💡 SUGG...
(SINCE_THEN_COMMA)
📍 Affects 2 files
docs/tickets/892/review.md#L28-L30(this comment)docs/tickets/892/review.md#L55-L59docs/tickets/892/review.md#L70-L89docs/tickets/892/tasks.md#L41-L47
| @@ -0,0 +1,124 @@ | |||
| # #892: Adopt MOJ Frontend mojFilter component across the six hand-rolled filter panels | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Standardise the panel count across the ticket records.
The documents call the scope “six” while they enumerate seven pages, including jurisdiction-data-list. This can cause one migrated page to be omitted from acceptance checks.
docs/tickets/892/ticket.md#L1-L1: update the title and related “six” references, or explicitly definejurisdiction-data-listas an additional in-scope page.docs/tickets/892/plan.md#L1-L15: align the strategy with the seven page entries in Lines [159-192].docs/tickets/892/tasks.md#L17-L24: replace “six pages” with the final scope.docs/tickets/892/review.md#L7-L7: align the summary and acceptance tally with the final scope.
📍 Affects 4 files
docs/tickets/892/ticket.md#L1-L1(this comment)docs/tickets/892/plan.md#L1-L15docs/tickets/892/tasks.md#L17-L24docs/tickets/892/review.md#L7-L7
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
🎭 Playwright E2E Test Results82 tests 52 ✅ 6m 2s ⏱️ Results for commit 8d5a454. ♻️ This comment has been updated with latest results. |
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
|
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
2 participants
Jira link
#892
Change description
Adopts MOJ Frontend mojFilter component
closes #892
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests