Skip to content

Comments

fix: Add optional close button control to Modal component#413

Merged
ainsleyclark merged 2 commits intomainfrom
claude/fix-modal-close-button-oBOdt
Feb 14, 2026
Merged

fix: Add optional close button control to Modal component#413
ainsleyclark merged 2 commits intomainfrom
claude/fix-modal-close-button-oBOdt

Conversation

@ainsleyclark
Copy link
Contributor

Summary

Added a new showClose prop to the Modal component that allows consumers to control whether the close button is displayed, while maintaining backward compatibility with a default value of true.

Key Changes

  • Added showClose prop to ModalProps type with default value of true
  • Updated modal header rendering logic to independently control title and close button visibility
  • Modified header conditional to render when either title or showClose is present
  • Added margin-left: auto to close button styling to ensure proper alignment when title is absent
  • Updated component documentation with new prop description and comprehensive props list

Implementation Details

  • The close button can now be hidden independently of the title, allowing for flexible modal header layouts
  • The header element still renders when only the close button is needed (no title)
  • Close button alignment is improved with margin-left: auto to handle cases where there's no title to push it to the right
  • Full backward compatibility maintained - existing modals will continue to show the close button by default

https://claude.ai/code/session_018sfc6wyLi8es84KFQ74rJW

The close button was previously nested inside the `{#if title}` block,
so it only appeared when a title was provided. This refactors the header
so the close button always renders (controlled by a new `showClose` prop
that defaults to true), and the title `<h4>` is conditionally rendered
separately.

https://claude.ai/code/session_018sfc6wyLi8es84KFQ74rJW
@ainsleyclark ainsleyclark changed the title Add optional close button control to Modal component fix: Add optional close button control to Modal component Feb 14, 2026
@claude

This comment has been minimized.

@claude
Copy link

claude bot commented Feb 14, 2026

Review summary

  • Overall score: 8/10
  • Critical issues: 0
  • Warnings: 1
  • Suggestions: 2
  • Recommendation: ⚠️ Approve with comments

Clean, focused change that correctly decouples close button visibility from title presence. Backward compatibility is maintained. One minor UX concern and a couple of small improvements worth considering.

Critical issues 🔴

None

Warnings 🟡

1. No way to close the modal when showClose={false} and no external dismiss mechanism exists

If a consumer sets showClose={false} without providing a separate dismiss trigger (e.g. a custom button in children or handling the backdrop/Escape elsewhere), the modal becomes impossible to close. The onClose callback is only wired up to the close button and is not triggered by backdrop clicks or keyboard events in the current implementation. Consider documenting this responsibility explicitly in the prop description, e.g. "When false, the caller is responsible for providing an alternative dismiss mechanism."

Suggestions 🟢

1. margin-left: auto applied unconditionally to __close

The style is added to the base __close selector, so it applies even when a title is present. It works in the current flex layout, but the intent is specifically for the titleless case. A more explicit approach — modifier class or :not selector — would make the alignment intent clearer and reduce risk if the header gains additional content in future.

2. No tests added

No test file currently exists for Modal.svelte, which is consistent with the repo state, but the new conditional branches (showClose={false}, close-only header with no title, etc.) are untested. Even a minimal vitest test covering the two new states would reduce regression risk for a public API change.

@codecov
Copy link

codecov bot commented Feb 14, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.94%. Comparing base (7f6b060) to head (d01692e).
⚠️ Report is 500 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #413      +/-   ##
==========================================
+ Coverage   64.59%   69.94%   +5.35%     
==========================================
  Files         154      185      +31     
  Lines        6064     7394    +1330     
==========================================
+ Hits         3917     5172    +1255     
+ Misses       2064     2025      -39     
- Partials       83      197     +114     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ainsleyclark ainsleyclark merged commit 16c4e91 into main Feb 14, 2026
6 checks passed
@ainsleyclark ainsleyclark deleted the claude/fix-modal-close-button-oBOdt branch February 14, 2026 19:55
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.

2 participants