Skip to content

fix: editing tag alignment and post-save redirect for budget lines tab - #6160

Open
Santi-3rd wants to merge 9 commits into
mainfrom
OPS-6013/bug-and-feedback-fixes
Open

fix: editing tag alignment and post-save redirect for budget lines tab#6160
Santi-3rd wants to merge 9 commits into
mainfrom
OPS-6013/bug-and-feedback-fixes

Conversation

@Santi-3rd

Copy link
Copy Markdown
Contributor

What changed

Three related UI/UX fixes for the Grants & Budget Lines and SCs & Budget Lines tabs on the agreement detail page.

DetailsTabs.jsx

  • Corrected tab label from "Grant & Budget Lines" to "Grants & Budget Lines" (pluralized to match the SC tab convention).

AgreementBudgetLinesHeader.jsx

  • Added margin-top-6 to the budget lines header container for consistent vertical spacing.

GrantNumberForm.jsx / ServicesComponentForm.jsx

  • The "Editing…" tag (pen icon + text) was vertically centered (flex-align-center) relative to the full FormHeader block (heading + details text), causing it to float below the heading. Changed to flex-align-start so the tag aligns with the heading row instead. Added small bottom padding so the tag doesn't crowd the content below.

CreateBLIsAndSCs.hooks.js

  • After a successful save from the Grants & Budget Lines or SCs & Budget Lines tab, the success alert was redirecting to /agreements/${id} (Agreement Details tab). Changed both redirect paths to /agreements/${id}/budget-lines so the user lands back on the tab they were editing. Blocker-modal navigation (savedViaModal) is unchanged.

Issue

#6013

How to test

  1. Open an existing grant agreement in edit mode and navigate to the Grants & Budget Lines tab.
    • Confirm the tab label reads "Grants & Budget Lines" (plural).
  2. Click Edit on a grant number row to open the edit form.
    • Confirm the "Editing…" tag (pen icon) appears inline with the "Edit Grant Numbers" heading, not centered below it.
  3. Make a change and click Save Changes.
    • Confirm the success banner appears and the page redirects to the Grants & Budget Lines tab (not the Agreement Details tab).
  4. Repeat steps 2–3 for a contract agreement on the SCs & Budget Lines tab to verify the same behavior for ServicesComponentForm.

Frontend unit tests:

cd frontend
bun run test --watch=false

A11y impact

  • No accessibility-impacting changes in this PR
  • Accessibility changes included and validated against WCAG 2.1 AA intent
  • Any temporary suppression includes A11Y-SUPPRESSION metadata (owner, expires, rationale)

Storybook

  • N/A — change is page-specific or non-visual

Screenshots

Screenshots should be added by the author to show the before/after of the "Editing…" tag alignment and the post-save redirect destination.

Definition of Done Checklist

  • OESA: Code refactored for clarity
  • OESA: Dependency rules followed
  • Automated unit tests updated and passed
  • Automated integration tests updated and passed
  • Automated quality tests updated and passed
  • Automated load tests updated and passed
  • Automated a11y tests updated and passed
  • Automated security tests updated and passed
  • 90%+ Code coverage achieved
  • Form validations updated

Links

N/A

@weimiao67 weimiao67 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.

Found and resolved 2 issues:

Finding 1 — handleFinancialSnapshotChanges redirect not updated (line 586)

The showSuccessMessage redirect was correctly updated to /budget-lines, but handleFinancialSnapshotChanges at line 586 still uses the old path:

redirectUrl: `/agreements/${selectedAgreement?.id}`  // missing /budget-lines

This is masked in the normal save flow because showSuccessMessage runs afterward and overwrites the alert. But on the suppressSuccessAlert=true path (batch/saveTrigger save), showSuccessMessage is skipped and this stale redirect fires, sending the user to the Agreement Details tab instead of Budget Lines.

@rajohnson90

Copy link
Copy Markdown
Contributor

Below is what my AI review found. I will start fixing these myself, starting from issue 1 but feel free to mention if you want to take any others.

  1. blocker.nextLocation doesn't exist (CreateBLIsAndSCs.hooks.js:652) — the "fix" reads a property react-router's Blocker never had (should
    be blocker.location, used correctly elsewhere in the same file). The redirect-after-save path only works today because of a redundant
    navigate() call elsewhere.
  2. Alignment fix in GrantNumberForm.jsx:77 targets the wrong container and papers over it with a magic padding-bottom-6 that will break
    again with different content length.
  3. AgreementBudgetLinesHeader.jsx:46 — unconditional margin-top-6 added to a header shared by two call sites, risking doubled spacing in
    one of them.
  4. Same "Editing..." tag markup duplicated across 3 components, fixed inconsistently in 2 and untouched in the 3rd.
  5. Redirect URL string literal now hardcoded 3x in one file — the same duplication pattern that caused this bug.
  6. The test mock's useBlocker shape doesn't match react-router's real API, so it can't catch here's a note #1.

@rajohnson90

Copy link
Copy Markdown
Contributor

Follow-up fixes from a code review of PR #6160, covering two areas:

Budget lines redirect bug

  • Replaced blocker.nextLocation — a property that doesn't exist on react-router's Blocker object (the real one is .location) — with
    blocker.location?.pathname at all 3 call sites plus a useCallback dependency in CreateBLIsAndSCs.hooks.js. This was a dead reference that silently
    broke the post-save redirect when a user saved via the "unsaved changes" modal.
  • Extracted the repeated /agreements/${id}/budget-lines string into a getBudgetLinesUrl(agreementId) helper, used at all 4 call sites in that file.
  • Fixed the useBlocker test mock to match react-router's real shape (location: { pathname } instead of a bare nextLocation string), and added a
    regression test asserting the redirect actually targets the blocker's pending destination when saved via the modal.

Duplicated "Editing…" indicator

  • The pen-icon + "Editing…" tag was copy-pasted with inconsistent alignment fixes (magic padding-bottom-6/padding-bottom-4 hacks) across
    GrantNumberForm.jsx, ServicesComponentForm.jsx, and AgreementDetailHeader.jsx.
  • Extracted a shared EditingIndicator component and gave FormHeader a new actions slot so it renders alongside the heading (rather than against the
    whole heading+details block, which is what caused the alignment bugs).
  • All three components now render the same indicator; the magic padding is gone.
  • Added test coverage for FormHeader's new actions prop (including a guard against a falsy actions leaking "false" into the DOM).

Self-review

Ran two adversarial reviews against these changes. Caught and fixed one real regression (a margin-top-6 layout fix I made earlier was reverted after
verifying it broke spacing at the wizard call site) and one design gap (FormHeader's actions slot was pushing wrapper-div duplication into callers —
now owned internally by FormHeader).

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants