Skip to content

fix: clickable step indicator, form discard guard, shared card grid, shared pagination - #1623

Merged
mftee merged 2 commits into
CodeGirlsInc:mainfrom
Ogunmepon:fix/step-indicator-discard-guard-grid-pagination
Sep 28, 2026
Merged

mftee merged 2 commits into
CodeGirlsInc:mainfrom
Ogunmepon:fix/step-indicator-discard-guard-grid-pagination

Conversation

@Ogunmepon

Copy link
Copy Markdown

Summary

Four defects, three of which share one root cause: the same behaviour implemented several times, slightly differently each time, so it drifted.

Issue Fix
#1511 Real buttons on the step indicator, gated to steps the user may revisit
#1512 Discard guard that only fires when there is something to lose
#1513 One shared ShipmentCardGrid for both lists
#1514 One shared Pagination for all three admin pages

#1511 — step indicator was decorative

The four-step progress indicator's circles were plain <div>s with no onClick and no button semantics, so clicking a completed step did nothing and the only way back was '← Back' one step at a time.

Each step is now a real <button type="button"> — the crux of the issue, since a clickable div with a bolted-on onClick and tabIndex is still not a control.

Eligibility was derived from how the form already works rather than invented. It advances only through advance(), which calls trigger(stepFields[step]) first, so a step index is only ever reached after that step's own fields validated. I track furthestStep (max index reached, monotonic) and step i is interactive iff i < furthestStep && i !== step:

  • a completed step is clickable and jumps there;
  • the current step is disabled rather than a focusable no-op that pretends to be a control;
  • an unvisited step is disabled;
  • previously-visited steps stay clickable after you jump backwards.

No step can be skipped past its own validation.

The circles and bars look identical, but the state is now actually exposed: a <nav> + <ol>, aria-current="step" on the current step, and per-state aria-labels ("… — completed" / "… — current step" / "… — not yet available") so a screen-reader user can tell the three states apart. The purely decorative connecting bars, which had no such treatment, are now aria-hidden="true".

#1512 — Cancel/Back silently discarded all entered data

Step-0 Cancel called router.back() immediately, with no route-change or beforeunload guard anywhere in the form, so a shipper who had filled in route, cargo and pricing data lost everything on one accidental click.

Only prompt when there is something to lose. "Dirty" is deliberately not react-hook-form's isDirty, which compares against defaultValues — here those are seeded from query params, so a pre-filled draft would look pristine while still being fully losable. A field is dirty when its value is neither empty nor its default (only currency has one, 'USD'), checked across all ten fields of all four steps via form.watch(), plus one evaluation on mount to catch prefilled drafts.

The dialog is role="dialog" aria-modal="true", labelled by its heading, focus moved to "Keep editing", Escape dismisses, focus returns to the Cancel button — reusing the markup and the Escape/focus pattern already used on the disputes page and in DocumentViewer/mobile-nav.

I checked Next 15.5.9 for a better mechanism before building this: there isn't one. unstable_usePrompt existed only in 14 canary and is gone in 15, and next/navigation exposes no blocker. A beforeunload guard is also in place for genuine page unload/refresh, showing the browser's generic wording — a platform constraint. What it does not cover is soft SPA back-navigation and in-app <Link> navigation, which are not unload events. I deliberately did not monkey-patch history: the sentinel-entry trick corrupts Back for Link-based navigation, and the issue's actual complaint is fully covered.

#1513 — two different grids for the same card list

marketplace used grid gap-4 sm:grid-cols-2 lg:grid-cols-3; shipments used the same ShipmentCard the same way but only sm:grid-cols-2, capping at two columns on desktop.

Extracted ShipmentCardGrid in components/shipment/, exporting the classes as a shared constant so the definitions cannot drift. marketplace's appearance is unchanged — it just sources the same constant, and its loading skeleton now uses it too, so the loaded and loading states cannot diverge either.

The issue's premise was stale, and the real state was worse. shipments/page.tsx had no grid at all — it delegated to ShipmentsInfiniteList, which rendered the cards in a plain single-column space-y-4 list. "My Shipments" was one column, not two. It is now on ShipmentCardGrid, which is the fix the issue actually wants, with an optional label prop so the list announces "My Shipments" / "My Jobs".

The only other ShipmentCard grid is the dashboard's "Recent Shipments" widget (grid gap-3 sm:grid-cols-2). I left it alone deliberately — it is a compact 2-up recent-items widget, not the shipment list, and folding it in would change its appearance beyond this issue's scope.

#1514 — three hand-rolled pagination footers

admin/users, admin/shipments and admin/disputes each implemented the same Prev/Next footer independently, and the copy had already drifted: "Showing X of Y users", "… shipments", and disputes' bare "Showing X of Y" with no noun at all.

I searched for an existing Pagination first — there is none in frontend/components or any vendored kit, so this is the first. components/ui/pagination.tsx encapsulates the drift-prone parts, and the noun is a required itemLabel prop so it cannot be omitted again. Page position moved from a CardDescription (present in two pages, absent in the third) into the shared caption: Showing 20 of 134 users (page 2 of 7).

The caption is itself the role="status" aria-live="polite" region, so a page change announces the new range and position without a second live region. <nav aria-label>, aria-current="page", correct disabled states at both ends, aria-label="Page N" on page buttons, windowed with ellipsis for long lists. Prev/Next keep their visible "Previous"/"Next" names, which is why the existing admin/shipments/page.test.tsx assertions still hold untouched.

Unifying needed no behaviour props, which is worth stating because it means nothing was silently unified away: all three already used limit: 20, all already reset to page 1 on filter change (disputes has no filters), all already gated the footer on totalPages > 1 (kept at the call site), and none has a delete, so there was no out-of-range case to reconcile.

Tests

25 new tests across shipments/new/page.test.tsx (16), components/ui/pagination.test.tsx (6) and components/shipment/shipment-card-grid.test.tsx (3) — 205 total, all passing.

Step indicator: real buttons, a completed step jumps, an unvisited one is disabled and does not jump, aria-current correct, keyboard reachable, Next/Back still work. Discard guard: untouched form cancels with no dialog, dirty form prompts, cancelling the dialog preserves all data, confirming navigates away, dialog is accessible. Pagination: caption includes the noun, Prev disabled on page 1, Next on the last, aria-current, accessible nav name.

admin/shipments/page.test.tsx and ShipmentsInfiniteList.test.tsx were not modified and still pass.

Verification

baseline this PR
frontend jest 13 failed / 27 passed suites 13 failed / 30 passed
12 failed / 167 passed tests 12 failed / 193 passed
tsc --noEmit 26 error lines identical (diff clean)
eslint (13 files) — clean

The failing set is unchanged; all 13 are pre-existing on main, several mocking modules that no longer exist (@/services/shipmentService) or importing pinia, which is not a dependency. Not fixed, and not used as templates. Backend untouched.

Closes #1511
Closes #1512
Closes #1513
Closes #1514

…one shared card grid, and one shared pagination

Four defects, three of them the same underlying problem: the same behaviour implemented
several times, slightly differently each time, so it drifted.

FE-182 — step indicator was decorative (CodeGirlsInc#1511)

`shipments/new` renders a four-step progress indicator whose circles were plain `<div>`s
with no `onClick` and no button semantics, so clicking a completed step did nothing and the
only way back was '← Back' one step at a time.

Each step is now a real `<button type="button">` — the crux of the issue, since a clickable
div with a bolted-on `onClick` and `tabIndex` is still not a control. Eligibility was derived
from how the form already works: it advances only through `advance()`, which calls
`trigger(stepFields[step])` first, so a step index is only ever reached after that step's own
fields validated. I track `furthestStep` (max index reached, monotonic) and make step `i`
interactive iff `i < furthestStep && i !== step`. That means a completed step is clickable
and jumps there, the current step is `disabled` rather than a focusable no-op, an unvisited
step is disabled, and previously-visited steps stay clickable after jumping backwards. No
step can be skipped past its own validation.

The circles and bars are visually unchanged, but the state is now exposed: a `<nav>` +
`<ol>`, `aria-current="step"` on the current step, and per-state `aria-label`s
("… — completed" / "… — current step" / "… — not yet available") so a screen-reader user can
tell the three states apart. The purely decorative connecting bars, which had no such
treatment, are now `aria-hidden="true"`.

FE-183 — Cancel/Back silently discarded all entered data (CodeGirlsInc#1512)

The step-0 Cancel called `router.back()` immediately, and there was no route-change or
`beforeunload` guard anywhere in the form, so a shipper who had filled in route, cargo and
pricing data lost everything on one accidental click.

Two parts:

- **Only prompt when there is something to lose.** "Dirty" is deliberately not
  react-hook-form's `isDirty`, which compares against `defaultValues` — here those are
  seeded from query params, so a pre-filled draft would look pristine while still being
  fully losable. A field is dirty when its value is neither empty nor its default (only
  `currency` has one, `'USD'`), checked across all ten fields of all four steps via
  `form.watch()`, plus one evaluation on mount to catch prefilled drafts.
- **The dialog itself** is `role="dialog" aria-modal="true"`, labelled by its heading, focus
  moved to "Keep editing", Escape dismisses, focus returns to the Cancel button. It reuses
  the markup and the Escape/focus pattern this repo already uses on the disputes page and in
  `DocumentViewer`/`mobile-nav`.

I checked Next 15.5.9 for a better mechanism first: there is none. `unstable_usePrompt`
existed only in 14 canary and is gone in 15, and `next/navigation` exposes no blocker. A
`beforeunload` guard is also in place for genuine page unload/refresh, with the browser's
generic wording — a platform constraint, not an oversight. What it does **not** cover is soft
SPA back-navigation and in-app `<Link>` navigation, which are not unload events. I
deliberately did not monkey-patch `history`: the sentinel-entry trick corrupts Back for
Link-based navigation, and the issue's actual complaint — the Cancel button — is fully
covered.

FE-184 — two different grids for the same card list (CodeGirlsInc#1513)

`marketplace` used `grid gap-4 sm:grid-cols-2 lg:grid-cols-3`; `shipments` used the same
`ShipmentCard` the same way but only `sm:grid-cols-2`, capping at two columns on desktop.

Extracted `ShipmentCardGrid` in `components/shipment/`, exporting the classes as a shared
constant so the definitions cannot drift. `marketplace`'s markup and appearance are
unchanged — it just sources the same constant, and its **loading skeleton** now uses it too,
so the loaded and loading states cannot diverge either.

The issue's premise turned out to be stale, and the real state was worse: `shipments` had no
grid at all, delegating to `ShipmentsInfiniteList`, which rendered the cards in a plain
single-column `space-y-4` list. "My Shipments" was **one** column, not two. It is now on
`ShipmentCardGrid`, which is the fix the issue actually wants, with an optional `label` prop
so the list announces "My Shipments" / "My Jobs".

The only other `ShipmentCard` grid is the dashboard's "Recent Shipments" widget
(`grid gap-3 sm:grid-cols-2`). I left it alone deliberately: it is a compact 2-up
recent-items widget, not the shipment list, and folding it in would change its appearance
beyond the issue's scope.

FE-185 — three hand-rolled pagination footers (CodeGirlsInc#1514)

`admin/users`, `admin/shipments` and `admin/disputes` each implemented the same Prev/Next
footer independently — own `page` state, own disabled logic, own caption — and the copy had
already drifted: "Showing X of Y users", "… shipments", and disputes' bare "Showing X of Y"
with no noun at all.

I searched for an existing `Pagination` first; there is none in `frontend/components` or any
vendored kit, so this is the first. The new `components/ui/pagination.tsx` encapsulates the
drift-prone parts, and the noun is a **required** `itemLabel` prop so it cannot be omitted
again. Page position moved from a `CardDescription` (present in two pages, absent in the
third) into the shared caption: `Showing 20 of 134 users (page 2 of 7)`.

The caption is itself the `role="status" aria-live="polite"` region, so a page change
announces the new range and position without a second live region. `<nav aria-label>`,
`aria-current="page"`, correct disabled states at both ends, `aria-label="Page N"` on page
buttons, windowed with ellipsis for long lists. Prev/Next keep their visible "Previous"/
"Next" names so the existing `admin/shipments/page.test.tsx` assertions still hold.

Unifying needed no behaviour props, which is worth stating: all three already used
`limit: 20`, all already reset to page 1 on filter change (disputes has no filters), all
already gated the footer on `totalPages > 1` (kept at the call site), and none has a delete,
so there was no out-of-range case to reconcile.

Tests: 25 new across 3 suites, 205 total, all passing. `admin/shipments/page.test.tsx` and
`ShipmentsInfiniteList.test.tsx` were not modified and still pass. Full frontend jest keeps
the failing set unchanged (13 suites / 12 tests, all pre-existing) with passing tests up
from 167 to 193. `tsc --noEmit` is byte-identical and eslint is clean on all 13 touched files.

Closes CodeGirlsInc#1511
Closes CodeGirlsInc#1512
Closes CodeGirlsInc#1513
Closes CodeGirlsInc#1514
@vercel

vercel Bot commented Sep 27, 2026

Copy link
Copy Markdown

@Ogunmepon is attempting to deploy a commit to the Mftee's projects Team on Vercel.

A member of the Team first needs to authorize it.

@drips-wave

drips-wave Bot commented Sep 27, 2026

Copy link
Copy Markdown

@Ogunmepon Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

…scard-guard-grid-pagination

# Conflicts:
#	frontend/app/(dashboard)/shipments/new/page.test.tsx
#	frontend/app/(dashboard)/shipments/page.tsx
@mftee
mftee merged commit 81a3dc9 into CodeGirlsInc:main Sep 28, 2026
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment