fix: schedule 11 styling - #243
Conversation
paulushcgcj
left a comment
There was a problem hiding this comment.
The Schedule 11 presentation changes are well covered by the focused unit suite, and the consolidated column definition is a particularly readable choice. Before we merge, can we complete and record the manual Dev/Prod verification called out by the linked issue? The remaining visual behavior is not exercised by the tests. I also found one test whose name does not match the interaction it performs, which makes the sorting coverage harder to interpret.
Additional notes (not tied to a specific diff line):
- [frontend/src/components/schedule11/index.tsx] Before we merge, can we complete and record the manual Dev/Prod verification required by the linked issue? Its Definition of Done explicitly includes manual testing of all PRs in Dev and Prod, while this PR marks that work as not done and says the six visual changes have not been viewed rendered. The focused unit suite gives us good coverage, but it cannot validate the Carbon layout, responsive wrapping, heading spacing, totals-row hover, or numeric header alignment in a browser.
gpascucci
left a comment
There was a problem hiding this comment.
Approved! This is an exceptionally high-quality, comprehensive, and well-tested frontend polish pass for Schedule 11, @SScholefield.
- Complete Alignment & Polish: The column reordering, bold totals row on grey headers, and moving comments to its own wider row under fields perfectly restore the legacy JSF behavior beautifully.
- Robust Column Sorting: The ASC -> DESC -> NONE sorting cycle is highly robust, and the new comprehensive unit tests in
Schedule11.test.tsx(all 354 tests green!) provide superb protection for sorting nulls, lexicographical values, and sort survival after mutations. - Resolution of Comments: Both of Paulo's comments have been beautifully resolved (the sorting-survival test flow has been fixed and aligned with the test name, and the manual Dev verification was completed and recorded with screenshots).
An absolutely stellar, production-ready frontend pass!
paulushcgcj
left a comment
There was a problem hiding this comment.
Focused Schedule 11 tests, lint, build, and diff checks are green.
The consolidated column definition keeps the table behavior and presentation aligned.
I did not identify any additional high- or medium-confidence changes beyond the existing threads.
Description
Eight UI fixes to the Schedule 11 (Basic Silviculture Costs) screen from a styling review against
the legacy app. No API, DTO, or backend changes — the entire diff is three files under
frontend/src/components/schedule11/, so it touches nothing owned by the other in-flight schedulebranches.
Each change was checked against the legacy JSF source rather than eyeballed, and most turned out to
be restoring behaviour the legacy screen already had:
addLocationPnlbeforelocationsDT(schedule11.xhtml:57,:204)Location | Biogeo/Subzone/Variant | ES | NAR(ha) | Actual Cost ($) | Planned Cost ($) | Total Act Plus Plan Cost ($) | Total/NAR(ha) | Comments | Actionsxhtml:208-364.schedule-1-other-costs__totals/.schedule-3-sub__totalsrule; same--cds-layer-accent-01token the data-table header usesxhtml:40-54); the app-wideContextBanneralready carries the working context on every page.schedule-3__section .cds--labelrows="3" cols="75"(xhtml:140-141) — far wider than the neighbouring inputs; ported as a75chcap so it scales with the fontp:columncarriessortByexcept Comments (xhtml:353) and Delete (xhtml:364)hideLabel; the Dropdown and ComboBox did not, so "Edit Enhanced" / "Edit Biogeo/Subzone/Variant" printed as visible text above every editing rowFixes #239
Type of change
How Has This Been Tested?
Schedule11.test.tsxgrows 29 → 37 tests. Full suite: 354/354 passing (20/20 files), pluseslint,stylelint,tsc --noEmitandvite buildall clean on the merged branch.New coverage for sorting:
aria-sortassertions at each stepwould order 120.5 first)
Plus one accessibility regression guard for #8: it asserts the Dropdown's label is present but
cds--visually-hidden, the ComboBox renders no visible label, and both controls are stillreachable via
getByRole('combobox', { name: … }). That last pair is what fails if anyone later"simplifies" this to
display: none.Checklist
Further comments
Not yet verified in a browser. Everything above is covered by unit tests, typecheck, lint and a
production build, but six of the eight changes are purely visual and none has been viewed rendered.
Worth a reviewer pulling the branch and looking at the page. The highest-risk spots are the numeric
header alignment (#7, see below) and the gap between the table title and the table (#2/#3).
Three deliberate deviations from legacy, called out so they're a conscious review decision:
state there is no way back to the document order the API returned, and the three-state cycle is
what Carbon's own
DataTabledoes. One-line revert inNEXT_DIRECTIONif the team prefers strictparity.
direction flip it would push empty rows above real data on the descending pass. I could not
confirm legacy's
nullSortOrderdefault with confidence, so this is a chosen behaviour ratherthan a ported one.
font-size: 12pxVerdana (main.css:174-175). Verdanaat 12px sits visually close to IBM Plex at 14px because of its larger x-height, so 14px is the
better perceptual match than Carbon's nominal 12px — but it is not a literal legacy value.
Two implementation notes reviewers may query:
h3, notTableContainer'stitleprop. Carbon routes that propthrough its
Section/Headingpair, which both sizes it atheading-03(20px) and picks its ownlevel (
h2) — so the page's two section headings disagreed on size and skipped a heading level. Itnow uses the same element and class as "Add New Location", making them the same size by
construction rather than by CSS override. The table keeps its accessible name via
aria-label.ComboBoxhas nohideLabelprop (unlikeDropdown), so hiding its label in table rows isdone by dropping
titleTextand passingaria-label— Carbon falls back toaria-labelon theinput exactly when
titleTextis absent. This keeps the accessible name that adisplay: nonewould have destroyed.
button, so
text-align: righton thethstops working and the numeric headers would drift leftof their right-aligned cells. Hence
.schedule-11__num .cds--table-sort { justify-content: flex-end }.Follow-up, deliberately out of scope: Schedules 1, 2, 3, 4 and 8 all pair a
TableContainer titlewith anh3and so have the same heading size/level mismatch as #2 above, and the totals-rowhover quirk (Carbon's row hover out-specifies a single-class totals background) exists on the two
sibling pages too. Both are left alone here to avoid conflicting with the other active schedule
branches — candidates for the consistency PR.
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: