Skip to content

MPDX-9815 - Add completion state to sub-steps in questionnaire components#1880

Open
zweatshirt wants to merge 2 commits into
mainfrom
step-status
Open

MPDX-9815 - Add completion state to sub-steps in questionnaire components#1880
zweatshirt wants to merge 2 commits into
mainfrom
step-status

Conversation

@zweatshirt

@zweatshirt zweatshirt commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Description

This adds a completion prop for SubStep array items based on which subsections of the form is completed. It is relatively simple since the form does not have many split-up sections.

https://jira.cru.org/browse/MPDX-9815

Testing

  • Go to NSO MPD Questionnaire
  • Ideally, start with a fresh form
  • Test the full flow and ensure that the substep circle icon fills in as the form is completed. Also good to double check here that the circular percentage bar works well.

Checklist:

  • I have given my PR a title with the format "MPDX-(JIRA#) (summary sentence max 80 chars)"
  • I have applied the appropriate labels (Add the label "Preview" to automatically create a preview environment)
  • I have run the Claude Code /pr-review command locally and fixed any relevant suggestions
  • I have requested a review from another person on the project
  • I have tested my changes in preview or in staging
  • I have cleaned up my commit history

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Bundle sizes [mpdx-react]

Compared against 5e57129

No significant changes found

@zweatshirt zweatshirt self-assigned this Jul 2, 2026
@zweatshirt zweatshirt added the Preview Environment Add this label to create an Amplify Preview label Jul 2, 2026
@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Preview branch generated at https://step-status.d3dytjb8adxkk5.amplifyapp.com

@zweatshirt zweatshirt changed the title Add completion state to sub-steps in questionnaire components MPDX-9815 - Add completion state to sub-steps in questionnaire components Jul 2, 2026
@zweatshirt zweatshirt marked this pull request as ready for review July 2, 2026 20:33
@zweatshirt

Copy link
Copy Markdown
Contributor Author

@canac Curious if I could get your opinion of the form's design in this preview! And if you had any constructive criticism over the UI/UX particularly. The form is ~95% done until stakeholders desire changes. After tying in the OneApp ministry list, it should be 100% unless I happened to miss anything.

@zweatshirt zweatshirt left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🤖 Multi-Agent Review — APPROVED WITH SUGGESTIONS

Verdict: ✅ APPROVED_WITH_SUGGESTIONS · 0 blockers · Risk: MEDIUM

5 agents (Architecture, Testing, Standards, UX, Financial-domain). This is a presentational / state-plumbing change — no auth, API, GraphQL, schema, or dependency surface. The Financial domain agent confirmed there is no financial-calculation code here (isStepComplete only checks field presence).

Resolved during review: the unused export on isFieldFilled was made module-private.

Non-blocking suggestions (posted inline): extract the shared 2-state dot renderer, add page-level wiring tests, and a couple of minor test robustness / edge-case items.

Considered and intentionally skipped: adding titleAccess a11y labels to the dots. MUI's titleAccess renders an SVG <title>, which browsers surface as a hover tooltip — unwanted on these dots — and the completion state is decorative/redundant next to the already-readable sub-step label (and the gap pre-dates this PR). If screen-reader labelling is wanted later, visually-hidden text is the right mechanism, not titleAccess.

CategoryListItemStyles,
} from 'src/components/HrTools/Shared/CalculationReports/Shared/styledComponents/StepsListStyles';

const StyledCategoryListItemIcon = styled(CategoryListItemIcon, {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Suggestion] This `StyledCategoryListItemIcon` re-implements the same two-state dot renderer (blue `CircleIcon` / gray `RadioButtonUnchecked`, `mpdxBlue.main` / `mpdxGrayDark.main`) that already exists in `GoalCalculator/SharedComponents/SectionList.tsx`. Consider extracting a shared 2-state dot renderer under `HrTools/Shared/` so the two flows stay in sync. Non-blocking. (The `styled()` + `shouldForwardProp` approach itself is fine.)

{
id: 'contact-information',
title: t('Contact Information'),
complete: isStepComplete(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Suggestion] The new per-page completion wiring (`isStepComplete(step, questionnaire)` → sub-step dot) has no integration test, and there is no `PersonalInformation.test.tsx`. A page passing the wrong step enum would still pass every existing test. Worth a page-level test asserting the Contact Information dot fills once `phoneNumber` (and, for married users, `spousePhoneNumber`) is present. The `SubStepList` presentational layer is already well covered.

expect(queryByTestId('CircleIcon')).not.toBeInTheDocument();
});

it('fills a sub-step dot blue once its data is complete', () => {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Suggestion] Minor test robustness: this asserts one `CircleIcon` and one `RadioButtonUncheckedIcon` render, but not that the *complete* row is the filled one — a `complete ? outlined : filled` inversion could still pass. A row-scoped assertion (e.g. `within(getByText('1. Section One').closest('li')).getByTestId('CircleIcon')`) would catch it. Also consider an all-complete case (all filled, zero outlined).

};

const isStepComplete = (
export const isStepComplete = (

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Suggestion] `isStepComplete` is now the exported entry point every page dot depends on and gained a new null guard. `stepCompletion.test.ts` exercises it only transitively via `getCompletionPercentage`. A direct unit test (`isStepComplete(step, null)` → `false`; per-step required-field cases including the married spouse-phone path) would pin the contract.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

AI Review Auto-Approval

Risk Level: MEDIUM (4/10)
Verdict: APPROVED_WITH_SUGGESTIONS (suggestions posted, no blockers)

This PR was auto-approved because:

  • The multi-agent AI review determined it is medium risk
  • No blocking issues were found
  • All suggestions have been posted as review comments for the developer to consider

If you believe this PR needs human review, dismiss this approval and request a review manually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Preview Environment Add this label to create an Amplify Preview

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant