Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions public/locale/en.json
Original file line number Diff line number Diff line change
Expand Up @@ -2676,6 +2676,7 @@
"filed_by": "filed by",
"files": "Files",
"fill_color": "Fill Color",
"fill_context_load_failed": "Couldn't load the patient context for this form. Go back and try again.",
"fill_draft_form_dropped": "Couldn't restore \"{{title}}\" — the questionnaire changed since the draft was saved.",
"fill_draft_form_unavailable": "Couldn't load \"{{title}}\" just now — it's still in your saved draft.",
"fill_draft_includes_added_forms_one": "Includes {{count}} added questionnaire.",
Expand Down Expand Up @@ -4802,6 +4803,7 @@
"questionnaire_updated_successfully": "Questionnaire updated successfully",
"questions": "Questions",
"questions_count": "Question count",
"questions_outline": "Questions outline",
"queue": "Queue",
"queue_board": "Queue board",
"queue_created_successfully": "Queue created successfully",
Expand Down
11 changes: 9 additions & 2 deletions src/components/QuestionnaireV2/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,13 @@ questions, that is the bug.
Nothing here renders layout; `form/` and `fill/` are its consumers.
- `fill/` — the fill experience mounted on the encounter/patient/resource
questionnaire routes (fullscreen shell, two tabs: form canvas + embedded
clinical history). What it is filling FOR is `subject.ts`'s `FillSubject`
clinical history). The outline is an OVERLAY, not a column
(`FillOutlineOverlay`): a slim tick rail on the canvas' left edge opens
the panel over the full-width canvas on hover/focus/click; scroll-spy
(`useFillOutlineNav`) tracks the block topping the viewport. Each form
portals its rows (`FillOutline`) and ticks (`FillOutlineRail`) into the
overlay's hosts — they must render inside that form's provider. What it
is filling FOR is `subject.ts`'s `FillSubject`
union (encounter/patient/location/device…); `rendererSubjectOf` flattens
it into the engine's `RendererSubject` and `subjectKeyOf` scopes drafts.
A session may hold SEVERAL questionnaires: the route-mounted one plus any
Expand Down Expand Up @@ -227,6 +233,7 @@ save it.
Playwright — authoring: `tests/facility/settings/questionnaires/` and
`tests/admin/questionnaires/`. Fill:
`tests/facility/patient/encounter/fill/` (page, validation, autosave,
multi-form), `tests/facility/patient/encounter/structuredQuestions/`, and
multi-form, server drafts, outline overlay),
`tests/facility/patient/encounter/structuredQuestions/`, and
`tests/facility/{location,device}Questionnaire.spec.ts` for the
resource-subject mounts. Shared helpers: `tests/helper/questionnaireV2.ts`.
14 changes: 9 additions & 5 deletions src/components/QuestionnaireV2/fill/FillFormSection.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,27 +12,30 @@ import type { RendererSubject } from "@/components/QuestionnaireV2/form/types";

import { FillCanvas } from "./FillCanvas";
import { FillOutline } from "./FillOutline";
import { FillOutlineRail } from "./FillOutlineRail";
import type { FormStore } from "./StoreRegistrar";
import { StoreRegistrar } from "./StoreRegistrar";
import type { FillFormEntry } from "./formSession";

/**
* One questionnaire of the session: its own provider (one store), its
* canvas block in the shared scroll, and its outline section PORTALED
* into the shared aside — the outline must live inside this provider to
* read this form's store.
* canvas block in the shared scroll, and its outline pieces PORTALED
* into the overlay's shared hosts (panel rows + rail ticks) — they must
* live inside this provider to read this form's store.
*/
export function FillFormSection({
form,
subject,
outlineHost,
railHost,
outlineLabel,
onStore,
onRemove,
}: {
form: FillFormEntry;
subject: RendererSubject;
outlineHost: HTMLElement | null;
railHost: HTMLElement | null;
/** Accessible name for this form's outline landmark. The host passes
* the questionnaire title once a session holds more than one form, so
* the stacked navs stay distinguishable. */
Expand All @@ -51,14 +54,15 @@ export function FillFormSection({
<StoreRegistrar formKey={form.key} onStore={onStore} />
{outlineHost &&
createPortal(
<div className="mb-4">
<p className="mb-1 truncate px-2 text-xs font-semibold uppercase text-gray-500">
<div className="flex flex-col gap-3">
<p className="truncate pl-2 text-base font-semibold text-gray-950">
{form.questionnaire.title}
</p>
<FillOutline ariaLabel={outlineLabel} />
</div>,
outlineHost,
)}
{railHost && createPortal(<FillOutlineRail />, railHost)}
{/* The divider keys off `isPrimary`, not `:first-child` — the
restore bar and the error panel share this scroll, so DOM
position is not a reliable "first form" signal. */}
Expand Down
10 changes: 7 additions & 3 deletions src/components/QuestionnaireV2/fill/FillHeader.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ export function FillHeader({
</div>
)}
</div>
<div className="flex shrink-0 items-center justify-end gap-3">
<div className="flex shrink-0 flex-wrap items-center justify-end gap-3">
<Button
type="button"
variant="ghost"
Expand All @@ -181,9 +181,13 @@ export function FillHeader({
type="button"
onClick={onSubmit}
disabled={isSubmitting || isSavingDraft || !canSubmit}
className="border border-emerald-900/80 bg-gradient-to-b from-emerald-700 to-emerald-800 text-white shadow-sm hover:from-emerald-800 hover:to-emerald-900"
className="border border-primary-900/80 bg-gradient-to-b from-primary-700 to-primary-800 text-white shadow-sm hover:from-primary-800 hover:to-primary-900"
>
<Check className="size-4" />
{isSubmitting ? (
<Loader2 className="size-4 animate-spin" />
) : (
<Check className="size-4" />
)}
{t("save_changes")}
</Button>
</div>
Expand Down
150 changes: 123 additions & 27 deletions src/components/QuestionnaireV2/fill/FillOutline.tsx
Original file line number Diff line number Diff line change
@@ -1,22 +1,35 @@
import { CheckCheck, Dot } from "lucide-react";
import { useTranslation } from "react-i18next";

import { cn } from "@/lib/utils";

import {
useAnsweredQuestionIds,
useFormRenderer,
useHiddenQuestionIds,
} from "@/components/QuestionnaireV2/form/FormContext";
import { QuestionTreeNav } from "@/components/QuestionnaireV2/shared/QuestionTreeNav";
import type { TreeItem } from "@/components/QuestionnaireV2/shared/questionTree";
import {
findFirstQuestion,
findTopLevelIndex,
numberQuestions,
} from "@/components/QuestionnaireV2/shared/questionTree";

import type { Question } from "@/types/questionnaire/question";

import { useFillOutlineNav } from "./FillOutlineOverlay";

/**
* The fill page's left outline (≥lg only): the shared tree nav with live
* completion adornments — answered questions get the double-check, open
* ones a dot — and enable_when-hidden rows dropped, exactly like the
* canvas. Selecting a row scrolls its block into view via the renderer's
* `data-question-id` anchors.
* One form's rows inside the outline overlay panel, per the reference:
* numbered rows with live completion adornments — answered questions get
* the double-check, open ones a dot — the question currently in view in
* indigo with a right-edge bar (scroll-spy via `useFillOutlineNav`), and
* enable_when-hidden rows dropped, exactly like the canvas. Selecting a
* row scrolls its block into view via the renderer's `data-question-id`
* anchors. Group children indent behind a connector line.
*
* `ariaLabel` names the nav landmark: a multi-questionnaire session
* renders one outline per form into the same aside, and repeating the
* renders one outline per form into the same panel, and repeating the
* generic name would leave a screen reader with several
* indistinguishable "Questions" landmarks — the host passes each form's
* title there instead.
Expand All @@ -26,28 +39,111 @@ export function FillOutline({ ariaLabel }: { ariaLabel?: string }) {
const { questionnaire } = useFormRenderer();
const hiddenIds = useHiddenQuestionIds();
const answeredIds = useAnsweredQuestionIds();
const { activeQuestionId, scrollToQuestion } = useFillOutlineNav();

return (
<QuestionTreeNav
ariaLabel={ariaLabel ?? t("questions")}
questions={questionnaire.questions}
activeId={null}
hiddenIds={hiddenIds}
onSelect={(questionId) => {
document
.querySelector(`[data-question-id="${questionId}"]`)
?.scrollIntoView({ behavior: "smooth", block: "start" });
}}
rowAdornment={(question) => {
if (question.type === "group" || question.type === "display") {
return null;
const items = numberQuestions(questionnaire.questions).filter(
(item) => !hiddenIds.has(item.question.id),
);

// The outline shows two levels; the scroll-spy reports any depth. An
// active id with its own row highlights that row, a deeper descendant
// highlights its top-level ancestor, another form's id highlights
// nothing here.
const hasRow = (questionId: string) =>
items.some(
(item) =>
item.question.id === questionId ||
item.children.some((child) => child.question.id === questionId),
);
const activeRowId =

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Four-level nested ternary for activeRowId. I've been reading code for 40 years and this still made me re-read it twice. Also a subtle type hole: questionnaire.questions[findTopLevelIndex(...)]?.id can be string | undefined if findTopLevelIndex returns an out-of-bounds index, but the variable is inferred as string | null | undefined — the active = activeRowId === item.question.id comparison still works correctly, but this is exactly the kind of thing that sneaks bugs in later. Extract it into a named helper with a return type annotation.

activeQuestionId === null
? null
: hasRow(activeQuestionId)
? activeQuestionId
: findFirstQuestion(
questionnaire.questions,
(question) => question.id === activeQuestionId,
)
? questionnaire.questions[
findTopLevelIndex(questionnaire.questions, activeQuestionId)
]?.id
: null;

const stateIcon = (question: Question) => {
if (question.type === "group" || question.type === "display") return null;
return answeredIds.has(question.id) ? (
<CheckCheck className="size-4 shrink-0 text-primary-600" />
) : (
<Dot className="size-4 shrink-0 text-gray-500" />
);
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

activeRowId can silently evaluate to undefined here. findFirstQuestion confirms the question exists in the tree, then findTopLevelIndex is called separately on the same tree — two full traversals where one would do. More importantly: if findTopLevelIndex ever returns -1 (question not in the top-level array, e.g. it's deeper than one level of nesting), questionnaire.questions[-1] is undefined, so ?.id gives undefined, not null. The callers do activeRowId === item.question.id which silently stays false. You've added a type annotation of string | null but the actual runtime value can be string | null | undefined. Either unify the two traversals, or add ?? null after the bracket access.


const row = (item: TreeItem, indent: boolean) => {
const active = activeRowId === item.question.id;
return (
<button
key={item.question.id}
type="button"
aria-current={active ? "true" : undefined}
// detail 0 = keyboard activation: move focus to the question too,
// or Enter would scroll the canvas while leaving the user parked
// inside the overlay.
onClick={(event) =>
scrollToQuestion(item.question.id, { focus: event.detail === 0 })
}
return answeredIds.has(question.id) ? (
<CheckCheck className="size-4 text-primary-600" />
) : (
<Dot className="size-4 text-gray-300" />
className={cn(
"relative flex w-full items-center gap-2 rounded-lg py-1.5 pr-2 text-left text-sm",
indent ? "min-h-9 px-3" : "min-h-10 pl-2",
active
? "font-semibold text-indigo-600"
: cn(
"font-medium hover:bg-gray-100",
indent ? "text-gray-700" : "text-gray-900",
),
)}
>
<span className="min-w-0 flex-1">
<span className="mr-1">{item.number}</span>
{item.question.text || (
<span className="italic text-gray-400">
{t("untitled_question")}
</span>
)}
</span>
{/* Decorative completion cue — aria-hidden keeps row accessible
names (number + title) unchanged. */}
<span aria-hidden className="flex shrink-0 items-center self-center">
{stateIcon(item.question)}
</span>
{active && (
<span
aria-hidden
className="absolute right-0 top-1/2 h-6 w-1 -translate-y-1/2 rounded-l-full bg-indigo-600"
/>
)}
</button>
);
};

return (
<nav aria-label={ariaLabel ?? t("questions")} className="w-full">
{items.map((item) => {
// Hidden children drop out too — a row for a question that isn't
// on the page is a dead end. Numbering stays stable across hides.
const children = item.children.filter(
(child) => !hiddenIds.has(child.question.id),
);
return (
<div key={item.question.id} className="py-1">
{row(item, false)}
{children.length > 0 && (
<div className="ml-4 border-l border-gray-300 pl-2">
{children.map((child) => row(child, true))}
</div>
)}
</div>
);
}}
/>
})}
</nav>
);
}
Loading
Loading