OP-5317: expanded BLI row layout adjustment#5558
Conversation
…yout Co-authored-by: Copilot <copilot@github.com>
…tments Co-authored-by: Copilot <copilot@github.com>
…S to module format
…d data Co-authored-by: Copilot <copilot@github.com>
…item tables Co-authored-by: Copilot <copilot@github.com>
There was a problem hiding this comment.
Pull request overview
Frontend updates to make expanded budget line item (BLI) rows more consistent with Figma by refactoring layout and consolidating “Created by/on” display, plus a small table alignment adjustment.
Changes:
- Refactors expanded-row layouts to use USWDS grid classes and consolidates “Created by” + “Created on” into a single
<dd>block in several expanded rows. - Adds table-level styling hook (
className) and averticalAlignToptable class to address cell vertical alignment. - Adds/extends procurement shop display in some expanded rows and updates one related test.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/pages/agreements/approve/ApproveAgreement.hooks.js | Updates budget line preview shaping for procurement shop change requests (sets agreement.procurement_shop). |
| frontend/src/components/UI/TableRowExpandable/TableRowExpandable.module.css | Adds CSS-module styling for expanded rows. |
| frontend/src/components/UI/TableRowExpandable/TableRowExpandable.jsx | Applies new expandedRow class to the expanded <tr>. |
| frontend/src/components/UI/Table/table.module.css | Adds verticalAlignTop table styling helper. |
| frontend/src/components/UI/Table/Table.jsx | Adds optional className prop to allow table-level styling from callers. |
| frontend/src/components/CANs/CANFundingReceivedTable/CANFundingReceivedTableRow.jsx | Consolidates created-by/on markup in the expanded section. |
| frontend/src/components/CANs/CANBudgetLineTable/CANBudgetLineTableRow.jsx | Adjusts expanded-row layout + created-by/on consolidation. |
| frontend/src/components/CANs/CANBudgetLineTable/CANBudgetLineTable.jsx | Applies verticalAlignTop table class. |
| frontend/src/components/BudgetLineItems/BudgetLinesTable/BLIRow.jsx | Updates expanded-row created-by/on markup and grid column sizing. |
| frontend/src/components/BudgetLineItems/BLIReviewTable/BLIReviewRow.jsx | Consolidates created-by/on and adds Procurement Shop to expanded content. |
| frontend/src/components/BudgetLineItems/BLIDiffTable/BLIDiffRow.test.jsx | Updates test to assert presence of “Procurement Shop” label in expanded content. |
| frontend/src/components/BudgetLineItems/BLIDiffTable/BLIDiffRow.jsx | Consolidates created-by/on and adds Procurement Shop to expanded content. |
| frontend/src/components/BudgetLineItems/AllBudgetLinesTable/AllBudgetLinesTable.jsx | Applies verticalAlignTop table class. |
| frontend/src/components/BudgetLineItems/AllBudgetLinesTable/AllBLIRow.jsx | Refactors expanded-row layout to USWDS grid and adjusts content blocks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fpigeonjr
left a comment
There was a problem hiding this comment.
Review — OP-5317: Expanded BLI Row Layout Adjustment
Summary
Clean frontend-only PR with good intent — grid layout refactor, "Created by"/"Created on" consolidation, Procurement Shop additions to diff/review rows, and a vertical alignment fix. A few issues need resolution before merge.
🔴 Accessibility Checkbox Mismatch (Medium)
The PR checks "No accessibility-impacting changes", but the consolidation of "Created by" / "Created on" into a single <dd> across six components is a structural change to <dl> semantics. The original markup had two proper <dt>/<dd> pairs for each field:
<!-- Before -->
<dt>Created on</dt>
<dd>…clock icon… July 26, 2024</dd>The new markup drops the <dt> for the date and nests it as an anonymous <div> inside the "Created by" <dd>:
<!-- After -->
<dt>Created by</dt>
<dd>
<div>John Doe</div>
<div>…clock… July 26, 2024</div> <!-- no semantic label -->
</dd>Screen readers traversing the definition list will associate only "Created by" as the term for the entire block — the creation date has no accessible label. Please either retain a <dt>Created on</dt> (visually hidden if needed), wrap the date in <time aria-label="Created on">, or update the A11Y checkbox and document the tradeoff.
🟡 verticalAlignTop Applied to Only 2 of 5 Affected Tables (Medium)
The new verticalAlignTop class is applied to AllBudgetLinesTable and CANBudgetLineTable but not to:
BudgetLinesTable(uses the updatedBLIRow.jsx)BLIReviewTable(uses the updatedBLIReviewRow.jsx)BLIDiffTable(uses the updatedBLIDiffRow.jsx)
If the vertical alignment issue exists in the first two, it likely exists in the other three as well. Please apply consistently or add a comment explaining why these tables are intentionally excluded.
🟡 Potential Fee Display Bug in Approve Workflow (Medium)
ApproveAgreement.hooks.js now sets updatedBudgetLine.agreement.procurement_shop to currentAwardingEntity / newAwardingEntity in both the before/after approval branches. getProcurementShopLabel() reads budgetLine.agreement?.procurement_shop?.current_fee?.fee for display — but per AgreementTypes.d.ts, current_fee is optional/nullable on ProcurementShop. If the /procurement-shops list endpoint does not populate current_fee, the label will show the correct abbreviation but "0%" for the fee rate, which would be misleading to approvers. Please verify the API response includes current_fee, or add a fallback: current_fee?.fee ?? fee_percentage.
🟡 padding-bottom: 0 !important Hits Expanded Row <td> (Low-Medium)
.verticalAlignTop td {
vertical-align: top;
padding-bottom: 0 !important;
}This selector targets all td elements, including the expanded row's <td colSpan={12}>. Bottom padding on the expanded content cell will be zeroed, which can look cramped against the next row. Consider narrowing the selector to skip expanded rows, or restore padding on the expanded td explicitly.
🟢 Missing Test for BLIReviewRow Procurement Shop (Low)
BLIDiffRow.test.jsx was updated to assert the new "Procurement Shop" label. BLIReviewRow.jsx received the same field addition but has no test file. Given the 90% coverage requirement, a test covering the expanded row in BLIReviewRow would be worth adding.
🔵 Non-blocking Notes
font-weight: 600on all<dd>in expanded rows (TableRowExpandable.module.css) bolds every value across the entire app's expanded rows — currency amounts, vendor names, descriptions, etc. Only elements explicitly markedtext-normalescape it. Confirm this matches design intent.CANFundingReceivedTableRowid fix (id="created-by-name"→ unique per row) is a correct and welcome change.- The
classNameprop onTableis a clean, safe extension.
Submitted by Frank's coding agent on his behalf.
|
@fpigeonjr, here are the explanations to your questions
|
fpigeonjr
left a comment
There was a problem hiding this comment.
Review Summary: PR #5558 - OP-5317: Expanded BLI Row Layout Adjustment
Status: APPROVED ✅
Changes Reviewed
This PR improves consistency across budget line item expanded rows by:
- Refactoring AllBLIRow to use USWDS grid system
- Consolidating 'Created by' + 'Created on' display in expanded rows
- Adding Procurement Shop field to BLIReviewRow and BLIDiffRow
- Fixing vertical alignment for tables with Agreement Name column
Follow-Up Verification (Commits aadcf21 & 7791ce1)
| Previous Concern | Resolution | Status |
|---|---|---|
| Missing BLIReviewRow tests | Added 173-line test file with 8 tests | ✅ Resolved |
| Incorrect colspan values | Fixed (12→10, 9→8) for proper table layout | ✅ Resolved |
| CSS selector syntax | Fixed .expandedRow dd :global(.text-normal) spacing | ✅ Resolved |
| verticalAlignTop only on 2 tables | Confirmed intentional (only tables with Agreement Name column) | ✅ Accepted |
| Procurement shop fee display | Confirmed not a bug — current_fee is computed property | ✅ Accepted |
Quality Checks
- ✅ ESLint: Clean
- ✅ Prettier: Clean
- ✅ BLIReviewRow tests: 8/8 passing
- ✅ Code follows project conventions
Minor Note
The A11Y checkbox was updated per author confirmation that the semantic
- changes match Figma design intent. Consider a future enhancement adding visually-hidden labels for screen readers.
Approved by Frank's coding agent on behalf of fpigeonjr.
|
🎉 This PR is included in version 1.372.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
What changed
This PR makes two categories of changes across the budget line item expanded rows:
Issue
#5317
How to test
The updated layout should match the Figma design
A11y impact
A11Y-SUPPRESSIONmetadata (owner, expires, rationale)Definition of Done Checklist