feat(components/modals): modal header close button is shown in modern theme

[Blackbaud-TrevorBurch]

AB#3695438

Summary by CodeRabbit

• New Features

  • Theme-aware styling for the modal close button (supports modern theme) and updated help button styling.
  • Customizable modal header alignment via a new CSS variable with a sensible default.

• Refactor

  • Simplified modal button display and spacing rules to improve styling consistency and maintainability.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c730a030-c5c7-4252-ba70-5c3c23006421

📥 Commits

Reviewing files that changed from the base of the PR and between 81e78f4 and 9bd298f.

📒 Files selected for processing (2)

• libs/components/modals/src/lib/modules/modal/modal.component.html
• libs/components/modals/src/lib/modules/modal/modal.component.scss

🚧 Files skipped from review as they are similar to previous changes (1)

• libs/components/modals/src/lib/modules/modal/modal.component.scss

---

📝 Walkthrough

Walkthrough

Modal component styling gained theme-aware button classes and a CSS variable for header alignment. Tests were simplified by removing a legacy assertion and adding clarifying comments; no runtime behavior changes.

Changes

Cohort / File(s)	Summary
Modal Template & Styling libs/components/modals/src/lib/modules/modal/modal.component.html, libs/components/modals/src/lib/modules/modal/modal.component.scss	Adds [skyThemeClass] binding for the close button to apply different classes per theme; introduces --sky-override-modal-header-align-items CSS custom property (fallback center) for header alignment; narrows a display rule to the help button and adds a margin rule for the close button.
Modal Testing (legacy) libs/components/modals/testing/src/legacy/modal-fixture.spec.ts, libs/components/modals/testing/src/legacy/modal-fixture.ts	Removes a spec asserting an error when the close button is missing; adds non-functional comments (Safety check, istanbul ignore else) around an existing visibility conditional in the fixture.
Package manifest package.json	Whitespace/manifest tidy (lines removed).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• fix(components/lookup): lookup and selections modals use correct footer button spacing in modern theme (#4229) #4230 — Adds theme-aware skyThemeClass bindings to modal buttons for modern vs default theme styling.

Suggested labels

risk level (author): 1

Poem

🐰
Hop, I tweak the modal's thread,
Buttons change their themed embroidery,
A tidy rule, a variable bed,
Tests made gentler, no harsh memory,
I nibble bugs, then bound with glee.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main feature: making the modal header close button visible in the modern theme, which is reflected in the HTML and SCSS changes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch modern-modal-close-button-2

📝 Coding Plan

• Generate coding plan for human review comments

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Tip

Migrating from UI to YAML configuration.

Use the @coderabbitai configuration command in a PR comment to get a dump of all your UI settings in YAML format. You can then edit this YAML file and upload it to the root of your repository to configure CodeRabbit programmatically.

[nx-cloud]

View your CI Pipeline Execution ↗ for commit b38250e

Command	Status	Duration	Result
nx build playground --baseHref=https://blackbau...	✅ Succeeded	2m 34s	View ↗
nx build code-examples-playground --baseHref=ht...	✅ Succeeded	5m 31s	View ↗
nx build integration --baseHref=https://blackba...	✅ Succeeded	1m 28s	View ↗

---

☁️ Nx Cloud last updated this comment at 2026-03-18 17:25:13 UTC

[coderabbitai]

🧹 Nitpick comments (2)

libs/components/modals/src/lib/modules/modal/modal.component.scss (1)

75-77: Consider aligning variable naming with the new selector scope.

Line 76 still uses --sky-override-modal-help-close-display, but this block now applies only to .sky-modal-btn-help. A help-specific alias would make intent clearer and avoid future confusion.

♻️ Optional aliasing refactor

 `@include` compatMixins.sky-default-overrides('.sky-modal') {
+  --sky-override-modal-help-display: var(--sky-override-modal-help-close-display, inline-block);
   --sky-override-modal-help-close-display: inline-block;
 }

 .sky-modal-btn-help {
-  display: var(--sky-override-modal-help-close-display, none);
+  display: var(
+    --sky-override-modal-help-display,
+    var(--sky-override-modal-help-close-display, none)
+  );
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@libs/components/modals/src/lib/modules/modal/modal.component.scss` around
lines 75 - 77, The CSS variable name --sky-override-modal-help-close-display is
misleading for the .sky-modal-btn-help selector; rename it to a help-specific
alias (e.g. --sky-override-modal-help-btn-display) and update the
.sky-modal-btn-help rule to use the new variable with the same fallback
(preserve "none"), then search and replace other usages of
--sky-override-modal-help-close-display across the codebase (or add a second
alias mapping) so all help-related components reference the new
--sky-override-modal-help-btn-display variable consistently.

libs/components/modals/testing/src/legacy/modal-fixture.ts (1)

105-106: Avoid suppressing coverage on this branch.

Line 106 hides coverage for the else path, but that path is still the explicit error contract when the close button is missing/hidden. Prefer keeping it covered.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@libs/components/modals/testing/src/legacy/modal-fixture.ts` around lines 105
- 106, Remove the "/* istanbul ignore else */" suppression on the safety-check
branch in modal-fixture.ts so the explicit else path remains covered, and add a
unit test that simulates the missing/hidden close button to exercise the else
branch (the code that throws the error when the close button is absent) — locate
the safety check around the close-button handling in ModalFixture (the method
that attempts to click/close the modal) and ensure the test asserts the expected
error is thrown so coverage includes that path.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@libs/components/modals/src/lib/modules/modal/modal.component.scss`:
- Around line 75-77: The CSS variable name
--sky-override-modal-help-close-display is misleading for the
.sky-modal-btn-help selector; rename it to a help-specific alias (e.g.
--sky-override-modal-help-btn-display) and update the .sky-modal-btn-help rule
to use the new variable with the same fallback (preserve "none"), then search
and replace other usages of --sky-override-modal-help-close-display across the
codebase (or add a second alias mapping) so all help-related components
reference the new --sky-override-modal-help-btn-display variable consistently.

In `@libs/components/modals/testing/src/legacy/modal-fixture.ts`:
- Around line 105-106: Remove the "/* istanbul ignore else */" suppression on
the safety-check branch in modal-fixture.ts so the explicit else path remains
covered, and add a unit test that simulates the missing/hidden close button to
exercise the else branch (the code that throws the error when the close button
is absent) — locate the safety check around the close-button handling in
ModalFixture (the method that attempts to click/close the modal) and ensure the
test asserts the expected error is thrown so coverage includes that path.

---

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bed5acf and 81e78f4.

📒 Files selected for processing (4)

• libs/components/modals/src/lib/modules/modal/modal.component.html
• libs/components/modals/src/lib/modules/modal/modal.component.scss
• libs/components/modals/testing/src/legacy/modal-fixture.spec.ts
• libs/components/modals/testing/src/legacy/modal-fixture.ts

💤 Files with no reviewable changes (1)

• libs/components/modals/testing/src/legacy/modal-fixture.spec.ts

[blackbaud-sky-build-user]

Storybook preview

Component Storybooks:

• action-bars-storybook
• ag-grid-storybook
• angular-tree-component-storybook
• avatar-storybook
• data-manager-storybook
• errors-storybook
• filter-bar-storybook
• flyout-storybook
• forms-storybook
• indicators-storybook
• layout-storybook
• lists-storybook
• lookup-storybook
• modals-storybook
• pages-storybook
• phone-field-storybook
• split-view-storybook
• tabs-storybook
• text-editor-storybook
• theme-storybook

Apps:

• code-examples-playground
• integration
• playground

[Blackbaud-SandhyaRajasabeson]

The logic lgtm, I have two questions:

1. harnesses: We currently have a function in the testing suite that sends the close action to the modal for testing purposes to close it. But with the close button now being available to modern modals, do we want to provide a close button click in the harness? It would have the same functionality as the closeTopModal function in the harness so I'm leaning more adding some context in the harness docs itself saying "simulating clicking the x button on the topmost modal"
2. Design has some linked spec stories to this story, does that include updating the images on docs after this PR goes out?

[Blackbaud-TrevorBurch]

The logic lgtm, I have two questions:

1. harnesses: We currently have a function in the testing suite that sends the close action to the modal for testing purposes to close it. But with the close button now being available to modern modals, do we want to provide a close button click in the harness? It would have the same functionality as the closeTopModal function in the harness so I'm leaning more adding some context in the harness docs itself saying "simulating clicking the x button on the topmost modal"
2. Design has some linked spec stories to this story, does that include updating the images on docs after this PR goes out?

Responses:

1. I'm not going to do this after a conversation offline due to it opening weirdness with out of order closing. We have the controller function for closing the top most.
2. I'll ping Adam but I would anticipate them doing that.

[Blackbaud-MattGregg]

One thing I noticed in reviewing to make the matching Figma component and update docs is there isn't a gap space to the left of the close button. We've put those in spots like these to avoid the text running into the button. Similar to box header, this could use var(--sky-space-text_action_gap-l);

[Blackbaud-TrevorBurch]

@Blackbaud-MattGregg fixed the gap token being missing.

[Blackbaud-MattGregg]

Looks good to me with the added gap. I don't see a way to actually approve it in this repo so just commenting.