Fix: [AEA-5081] - fixed skip link focus and updated trivyignore#1798
Fix: [AEA-5081] - fixed skip link focus and updated trivyignore#1798jonathanwelch1-nhs merged 16 commits intomainfrom
Conversation
|
This PR is linked to a ticket in an NHS Digital JIRA Project. Here's a handy link to the ticket: AEA-5081 |
jonathanwelch1-nhs
temporarily deployed
to
dev-pr
GitHub Actions
Inactive
jonathanwelch1-nhs
temporarily deployed
to
dev-pr
GitHub Actions
Inactive
jonathanwelch1-nhs
temporarily deployed
to
dev-pr
GitHub Actions
Inactive
jonathanwelch1-nhs
temporarily deployed
to
dev-pr
GitHub Actions
Inactive
packages/cpt-ui/src/App.tsx
Outdated
| const target = event.target as HTMLElement | ||
|
|
||
| if (target && target.id) { | ||
| const interactiveSelectors = [ |
There was a problem hiding this comment.
youve got this list in at least 2 places, best to condense into a single common place to avoid any issues if anything ever needs adding or removing.
|
|
||
| // Initial check: see if there's an active interactive element on page load | ||
| const activeElement = document.activeElement as HTMLElement | ||
| const hasActiveInteractiveElement = activeElement && |
There was a problem hiding this comment.
could this condition be tightened up any? theres a lot of moving parts to it
| target.tagName === "TEXTAREA" || | ||
| target.tagName === "SELECT" || | ||
| target.tagName === "BUTTON" || | ||
| target.hasAttribute("contenteditable") |
There was a problem hiding this comment.
can this condition be tightened/neatened up any?
| // Default behavior: if user hasn't interacted, focus skip link | ||
| if (!hasUserInteracted) { | ||
| e.preventDefault() | ||
| const skipLink = document.querySelector(".nhsuk-skip-link") as HTMLElement |
There was a problem hiding this comment.
this might be a little brittle if the css class name ever changes in the framework etc can we use an id that were in full control of instead?
| lastElement.focus() | ||
| } else { | ||
| e.preventDefault() | ||
| const skipLink = document.querySelector(".nhsuk-skip-link") as HTMLElement |
There was a problem hiding this comment.
skip link is being defined twice within this nested set of conditions, can it just be defined one before this block?
| } | ||
|
|
||
| // Remove listeners after handling first tab | ||
| document.removeEventListener("keydown", handleKeyDown) |
There was a problem hiding this comment.
these same set of 4 removals happen in 2 places, is it worth pulling them out into a little common function?
packages/cpt-ui/src/App.tsx
Outdated
| activeElement !== document.body && | ||
| activeElement.tagName !== "HTML" && | ||
| (activeElement.tagName === "INPUT" || | ||
| activeElement.tagName === "TEXTAREA" || activeElement.tagName === "SELECT")) { |
There was a problem hiding this comment.
can this condition be tightened up any?
packages/cpt-ui/src/App.tsx
Outdated
| document.removeEventListener("keydown", handleKeyDown) | ||
| const mainContent = document.querySelector("#main-content") || document.body | ||
| mainContent.removeEventListener("click", handleUserInteraction) | ||
| mainContent.removeEventListener("focusin", handleUserInteraction) |
There was a problem hiding this comment.
are these the same set of removals from the helper? if so can a common function work for all occasions your doing this?
packages/cpt-ui/src/App.tsx
Outdated
| document.removeEventListener("keydown", handleKeyDown) | ||
| document.removeEventListener("click", handleUserInteraction) | ||
| document.removeEventListener("focusin", handleUserInteraction) | ||
| const mainContent = document.querySelector("#main-content") || document.body |
There was a problem hiding this comment.
mainContent is defined twice, can it be moved up in scope and only defined once?
packages/cpt-ui/src/App.tsx
Outdated
| setTimeout(addDirectListeners, 100) | ||
|
|
||
| const mainContent = document.querySelector("#main-content") || document.body | ||
| mainContent.addEventListener("click", handleUserInteraction) |
There was a problem hiding this comment.
see other comments about having common functions for these add/remove listeners that are in several places
jonathanwelch1-nhs
temporarily deployed
to
dev-pr
GitHub Actions
Inactive
jonathanwelch1-nhs
temporarily deployed
to
dev-pr
GitHub Actions
Inactive
|
jonathanwelch1-nhs
temporarily deployed
to
dev-pr
GitHub Actions
Inactive
2 participants
Summary
fixed skip link focus and updated trivyignore plus updated title
https://nhsd-jira.digital.nhs.uk/browse/AEA-5081