fix: Add optional close button control to Modal component#413
fix: Add optional close button control to Modal component#413ainsleyclark merged 2 commits intomainfrom
Conversation
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
This comment has been minimized.
This comment has been minimized.
Review summary
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 Report✅ All modified and coverable lines are covered by tests. 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. 🚀 New features to boost your workflow:
|
Summary
Added a new
showCloseprop to the Modal component that allows consumers to control whether the close button is displayed, while maintaining backward compatibility with a default value oftrue.Key Changes
showCloseprop toModalPropstype with default value oftruetitleorshowCloseis presentmargin-left: autoto close button styling to ensure proper alignment when title is absentImplementation Details
margin-left: autoto handle cases where there's no title to push it to the righthttps://claude.ai/code/session_018sfc6wyLi8es84KFQ74rJW