Update Modal component to Svelte 5#820
Conversation
Switch Svelte 4 `export let` statements to Svelte 5
`let {...} = $props();`
Replace all fragment calls inside Modal elements by snippets.
Convert `on:click` to Svelte 5 `onclick`. Convert `on:close` to Svelte 5 `onclose`. Add `onclose` to props list, fixing NotesDialog.
Fix issue where label snippet is not rendered.
WalkthroughThis change updates how modal content is defined and rendered across several Svelte components. The approach shifts from using named slot fragments ( Changes
Sequence Diagram(s)sequenceDiagram
participant ParentComponent as Any Component (e.g., AudioPlaybackSpeed)
participant Modal as Modal.svelte
ParentComponent->>Modal: Pass props (id, content, label, addCSS, onclose)
Modal->>Modal: Render label if label() exists (removed in this update)
Modal->>Modal: Render content via content() function
Modal->>ParentComponent: Call onclose() when modal closes
Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
judah-sotomayor
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/lib/components/NoteDialog.svelte (1)
56-97: 💡 Verification agent🧩 Analysis chain
LGTM! Complete Svelte 5 Modal migration with proper event handling.
This file demonstrates the full migration pattern:
- Correct snippet syntax replacing slots
- Proper event handling update from
on:close={reset}toonclose={reset}prop- All conditional content and editing functionality preserved
The migration to Svelte 5 patterns is correctly implemented.
Since this is a breaking API change affecting the Modal component, please verify that all Modal component usages across the codebase have been updated to the new Svelte 5 pattern:
🏁 Script executed:
#!/bin/bash # Description: Verify all Modal component usages have been updated to Svelte 5 patterns # Expected: No old slot-based patterns should remain echo "=== Checking for old slot-based Modal patterns ===" rg -A 3 -B 1 '<svelte:fragment slot="content">' echo "=== Checking for old useLabel prop usage ===" rg -A 2 -B 1 'useLabel=' echo "=== Checking for old on:close event handlers on Modal ===" rg -A 2 -B 1 'on:close.*=' --type svelte echo "=== Verifying all Modal usages now use snippet pattern ===" rg -A 5 '<Modal' | rg -A 5 '\{#snippet'Length of output: 4956
Incomplete Svelte 5 Modal migration: remaining old slot-based patterns
The verification script shows several components still using the legacy
<svelte:fragment slot="content">API. Please update these to the new{#snippet content()}pattern and remove any old slot-based Modal props:• src/lib/components/ChapterSelector.svelte
• src/lib/components/LayoutOptions.svelte (two occurrences)
• src/lib/components/BookSelector.svelteAdditionally, ensure none of these files use the old
on:close={…}event syntax—replace with the Svelte 5onclose={…}prop.
🧹 Nitpick comments (1)
src/lib/components/Modal.svelte (1)
15-15: Consider adding prop validation for better developer experience.While the current implementation works, consider adding runtime validation to ensure
contentandlabelare functions when provided, especially since this is a breaking API change.let { id, addCSS = '', content, label, onclose } = $props(); + +// Validate function props +if (content && typeof content !== 'function') { + console.warn('Modal: content prop must be a snippet function'); +} +if (label && typeof label !== 'function') { + console.warn('Modal: label prop must be a snippet function'); +}Also applies to: 40-40
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/lib/components/AudioPlaybackSpeed.svelte(2 hunks)src/lib/components/CollectionSelector.svelte(2 hunks)src/lib/components/FontSelector.svelte(2 hunks)src/lib/components/Modal.svelte(2 hunks)src/lib/components/NoteDialog.svelte(2 hunks)src/lib/components/PlanStopDialog.svelte(2 hunks)src/lib/components/TextAppearanceSelector.svelte(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: lint
🔇 Additional comments (12)
src/lib/components/PlanStopDialog.svelte (1)
44-68: LGTM! Correct Svelte 5 Modal migration.The migration from slot-based content (
<svelte:fragment slot="content">) to snippet-based content ({#snippet content()}) follows the correct Svelte 5 pattern. The modal content and functionality are preserved unchanged.src/lib/components/FontSelector.svelte (1)
27-43: LGTM! Consistent Svelte 5 Modal migration.The migration correctly updates the Modal usage to Svelte 5 snippet syntax while preserving all existing functionality including the FontList component and action buttons.
src/lib/components/CollectionSelector.svelte (1)
66-106: LGTM! Proper Svelte 5 migration with complex content preserved.The migration correctly implements the snippet-based approach while maintaining the complex TabsMenu structure and layout options. All functionality appears to be preserved in the transition to Svelte 5.
src/lib/components/AudioPlaybackSpeed.svelte (2)
32-32: LGTM: Modal component migration looks correct.The removal of
useLabel={false}prop aligns with the new Modal API where label rendering is conditional based on the presence of thelabelprop.
33-56: LGTM: Snippet syntax correctly implemented.The migration from
<svelte:fragment slot="content">to{#snippet content()}is properly implemented and follows Svelte 5 conventions.src/lib/components/TextAppearanceSelector.svelte (2)
108-108: LGTM: Modal props correctly updated.The removal of
useLabel={false}and preservation ofaddCSSprop is correct for the new Modal API.
109-200: LGTM: Snippet migration properly implemented.The content has been correctly wrapped in a
{#snippet content()}block, maintaining all the existing functionality while adopting Svelte 5 conventions.src/lib/components/Modal.svelte (5)
6-11: LGTM: Prop documentation correctly updated.The prop documentation accurately reflects the new API with function-based content and label props.
15-15: LGTM: Props destructuring correctly implemented.The use of
$props()with destructuring and default values follows Svelte 5 conventions properly.
19-25: LGTM: Exported showModal function properly implemented.This provides programmatic control over the modal, which is essential for the component's functionality.
40-40: LGTM: Event binding correctly updated for Svelte 5.The direct
{onclose}prop binding is the correct approach for Svelte 5 event handling.
49-49: LGTM: Snippet rendering correctly implemented.The use of
{@render content?.()}with optional chaining is a good defensive programming practice.
Make `label` and `onclose` default to `null`.
|
I believe I recently removed the last place where "useLabel" was used (it caused problems and was not useful). I will remove it. |
|
A better branch name would have been |
* The last use of the label was removed in PR#816. Realistically, it was a bad idea.
2 participants
Per #819
Tested on all locations where Modal is used:
Label functionality is currently untested.
Not sure how to test that.
Summary by CodeRabbit