fix(schedule-6): legacy-faithful polish for the Schedule 6 page - #330
Conversation
✔️ PR Review & Verification: APPROVEDWe fetched 🧪 Local Verification Loop Results
🔍 Key Verification & Architectural Highlights
🚨 PR Validation Check FixedWe noticed that the PR Validate / Checks check was failing with a Conventional Commits syntax error:
This was caused by an accidental leading space at the very start of the PR title. We used the GitHub CLI to update the PR title to: GitHub Actions has automatically picked up this change and triggered a fresh validation run which is now passing perfectly. 🏁 Final VerdictThe changes in this PR represent high-integrity, meticulous CSS and React engineering. Visual hierarchy is vastly improved, redundant components are pruned, and the test suite has been elegantly expanded with high-coverage plural queries. Highly recommended for immediate merge! 🚀 |
There was a problem hiding this comment.
Pull Request Review: PR #330 (bcgov/nr-ilcr)
✅ Review Verdict: APPROVED
This is an exceptionally clean, well-reasoned, and legacy-faithful PR. It bridges the visual gap between the modern React-based Schedule 6 implementation and the legacy screen layouts without introducing any functional regressions or API-level churn.
Key Strengths & Polish Points:
-
DRY Duplicated Action Bars:
- Decomposing the action bars into a reusable
actionBar(includeAdd: boolean)helper is a great choice. It prevents code duplication while precisely mirroring legacy'ssaveButton0/saveButton1(top/bottom) split. - Gating
Addon the top bar only makes complete sense conceptually and holds parity with legacy.
- Decomposing the action bars into a reusable
-
UI Alignment & Layout Polish:
- Mismatched Label Alignment Fixed: Shifting
.schedule-6__fieldsfromalign-items: flex-end(bottom-aligned) toalign-items: start(top-aligned) is a subtle but critical fix. Because BR-02 always blanks either TFL or Supply Block, bottom-alignment pushed the empty fields' labels out of line. Top-alignment guarantees every label sits on the same baseline under any input combination. - Horizontally Compact Totals: Shifting the Totals region from the wide record-row grid to its own
flexcontainer is a great usability improvement. It keeps totals in a neat horizontal band instead of wrapping short values into vertical stacks. - Left-aligned dl values: Swapping the dl cells to
text-align: leftaligns them visually with their labels above, improving scanability.
- Mismatched Label Alignment Fixed: Shifting
-
Strong Verification (Tests Updated & Extended):
- The new assertions added to
Schedule6.test.tsx(likeSave and Check Status render on both barsandthe bottom bar's Save fires the same General Comment PUT as the top one) are extremely thorough. - Modifying all existing query selectors from
Edit Volume m³toVolume m³perfectly validates the removal of the redundant label prefixes.
- The new assertions added to
paulushcgcj
left a comment
There was a problem hiding this comment.
The Schedule 6 presentation changes match the stated legacy-faithful polish scope. The focused tests, lint, formatting, diff checks, and GitHub checks are clean, and I found no blocking or actionable issues. The added layout assertions are particularly useful because they cover both action-bar behavior and placement around General Comments.
Description
Nine presentation fixes to the Schedule 6 page (Story 8.3), each bringing the modern page closer to the legacy screen. No API calls, request bodies, validation rules, or state transitions change — every fix is layout, typography, or button placement. Three files touched: frontend/src/components/schedule6/{index.tsx,index.scss,tests/Schedule6.test.tsx}.
Save / Check Status on both bars — legacy carried saveButton0 + checkStatusButton0 above the schedule and saveButton1 + checkStatusButton1 again below the General Comment (schedule6.xhtml:222-229, :518-526). The page now renders one shared actionBar() twice, matching the duplicated-bar pattern Schedules 1 and 3 already use. Add rides the top bar only — it toggles the panel directly beneath it, and legacy's bottom bar carried no add control. The Save that sat beside the General Comment textarea moved onto the lower bar, where legacy put it.
Removed the Mill / Reporting Year / Status strip — Schedule 6 was the only schedule page painting one; siblings show none, and the working context already lives in the banner.
Dropped the "Edit " label prefix — editor inputs read TSA or TFL, Volume m³, etc., identical to the Add panel and to legacy, whose rows were always directly editable under those bare names. The labelPrefix prop is gone; idPrefix still keeps every input's id/htmlFor pairing unique.
Left-alignschedules (1/3/11) where it aids column scanning; these values sit in dl grid cells with the label directly above, so it only pushed each number away from its label. Same choice as schedule2.
Gave the derived read-outs Carbon field metrics — RMG and $ / m³ sit among Carbon inputs in the add/edit grid but had no Carbon metrics, so their labels rendered bold at the wrong size and their values landed on the wrong line. They now carry label-01 labels and body-compact-01 values, with min-height: 2rem and padding-inline-start: 1rem so the value sits on the inputs' line at the inputs' text inset — the read-only-vs-input indent documented in schedule1/index.scss:100-108, schedule2:25, schedule3:58.
Fixed a label that dropped out of line — .schedule-6__fields was bottom-aligned. BR-02 always blanks one of TFL / Supply Block, and an empty dd generates no line box, so the shorter field's label was pushed down. Top alignment pins every label to the row's first line regardless of an empty value, and stops a field that grows a validation message from shifting its neighbours.
Label/value distinction without bold — labels are now told apart by size and colour (12px label-01 in --cds-text-secondary) over a 14px primary value, with Carbon's standard $spacing-03 gap where there was none. Bold is gone from every dt, including the Totals labels and the Totals: prefix.
Totals in one horizontal band — the totals
was borrowing the record-row grid, whose minmax(10rem, 1fr) tracks are sized for rows of inputs and wrapped three short numbers into a vertical stack. It now has its owand both follow the active theme. Scoped to this page's accordion.- Bug fix (non-breaking change which fixes an issue)
- Updated existing tests
- New unit tests
- Manual tests (description below)
- I have performed a self-review of my own code
- I have commented my code, particularly in hard-to-understand areas
- My changes generate no new warnings
- I have added tests that prove my fix is effective or that my feature works
- New and existing unit tests pass locally with my changes
- Frontend
- Backend
- Analysis Workflow
- Merge Workflow
Fixes #288
Type of change
How Has This Been Tested?
869 frontend tests / 50 files pass, ESLint and Prettier clean, npm run build compiles the SCSS. The Schedule 6 suite is 84 (52 page + 32 validation), up 2.
Two new tests: one asserting both action bars render with Add on the top one only and the correct DOM ordering around the General Comment, one clicking the bottom Save and asserting it fires exactly one identical PUT /general-comments. Existing queries became plural (getAllByRole(...)[0], the Schedules 1/3 convention) now that Save and Check Status appear twice, and the editable: false test sweeps both bars rather than one.
The CSS items are unverifiable in jsdom — no layout engine — so those were confirmed against the compiled stylesheet (label weight 400, align-items: start, the --cds-layer-hover rule) and checked visually in the running dev stack.
Checklist
Thanks for the PR!
Deployments, as required, will be available below:
Please create PRs in draft mode. Mark as ready to enable:
After merge, new images are deployed in: