fix: #1789 add onclose prop#1790
Conversation
|
@shinokada is attempting to deploy a commit to the Themesberg Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedMore than 25% of the files skipped due to max files limit. The review is being skipped to prevent a low-quality review. 192 files out of 299 files are above the max files limit of 100. Please upgrade to Pro plan to get higher limits. You can disable this status message by setting the WalkthroughAdds optional onclose callbacks across components and types. Banner and Popper now accept and invoke onclose; Dropdown exposes onclose in its prop signature but does not forward it to Popper. Documentation and examples demonstrating onclose/ontoggle were added. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Consumer (app)
participant D as Dropdown
participant P as Popper
participant B as Banner
App->>D: <Dropdown onclose={handler}>
note right of D: Dropdown extracts `onclose` from props\nbut does NOT forward it to Popper
D->>P: Mount Popper (without forwarded onclose)
note over P: User toggles dropdown
P->>P: on_toggle(ev)
alt ev.newState == "closed"
P->>P: invoke internal `_onclose` if provided locally
P-->>App: (only if Popper received an onclose) calls onclose(ev)
else
P-->>D: update state
end
%% Banner flow (illustrative)
App->>B: <Banner onclose={bannerHandler}>
B->>B: user clicks close -> sets open = false
B->>App: calls bannerHandler(event)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
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. Comment |
|
@jjagielka Please check if this is ok for your side. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/lib/utils/Popper.svelte (1)
216-240: Update documentation comment to include the new prop.The component documentation comment at the end of the file should list the
oncloseprop for completeness.Apply this diff to update the documentation:
@prop onbeforetoggle: _onbeforetoggle @prop ontoggle: _ontoggle +@prop onclose: _onclose @prop class: className = ""src/lib/dropdown/Dropdown.svelte (1)
35-49: Update documentation comment to include the new prop.The component documentation comment should list the
oncloseprop for completeness.Apply this diff to update the documentation:
@prop activeUrl = "" @prop isOpen = $bindable(false) +@prop onclose @prop ...restProps
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lib/dropdown/Dropdown.svelte(2 hunks)src/lib/types.ts(2 hunks)src/lib/utils/Popper.svelte(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-22T08:37:27.996Z
Learnt from: mrh1997
PR: themesberg/flowbite-svelte#1442
File: src/lib/utils/Popper.svelte:144-145
Timestamp: 2024-11-22T08:37:27.996Z
Learning: In `src/lib/utils/Popper.svelte`, inconsistencies in event handling (e.g., using both `mouseenter`/`mouseleave` and `pointerenter`/`pointerleave`) may be intentional to minimize modifications and reduce the risk of new problems.
Applied to files:
src/lib/utils/Popper.svelte
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Playwright and Lint
🔇 Additional comments (3)
src/lib/types.ts (1)
2019-2019: LGTM! Type definition is correct.The
onclosecallback has been properly added toPopperPropswith the correct type signature(ev: TriggeredToggleEvent) => void, and it's correctly omitted fromHTMLAttributesto prevent conflicts.Also applies to: 2034-2034
src/lib/utils/Popper.svelte (1)
10-10: LGTM! Implementation follows the established pattern.The
onclosecallback is properly destructured and invoked when the popover transitions to the closed state. The implementation is consistent with the existingontoggleandonbeforetogglehandlers.Also applies to: 141-143
src/lib/dropdown/Dropdown.svelte (1)
10-10: LGTM! Prop forwarding is implemented correctly.The
oncloseprop is properly extracted from the component props and forwarded to thePoppercomponent, enabling the callback functionality for dropdown close events.Also applies to: 25-25
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/routes/docs-examples/components/dropdown/Onclose.svelte (1)
7-7: Consider documenting the event parameter or showing practical usage.The
onclosecallback receives an event parameter but doesn't use it. While this simple example demonstrates the feature, consider either:
- Showing a more practical use case as mentioned in issue #1789 (e.g., triggering a state update or request)
- Adding a comment explaining what event information is available
This would help users understand the full capabilities of the callback.
Example showing the event structure:
-<Dropdown simple onclose={(ev) => console.log('closed by onclose')}> +<Dropdown simple onclose={(ev) => console.log('Dropdown closed:', ev)}>Or a more practical example:
-<Dropdown simple onclose={(ev) => console.log('closed by onclose')}> +<Dropdown simple onclose={(ev) => { + console.log('Dropdown closed, triggering update...'); + // Example: triggerUpdate() or handleSelectionChange() +}}>src/routes/docs-examples/components/dropdown/Ontoggle.svelte (1)
7-11: LGTM! Consider adding a comment explaining ontoggle vs onclose.The implementation correctly demonstrates checking
ev.newState === 'closed'withontoggle. However, users might wonder when to useontoggleversusonclosesince both examples log on close.Consider adding a brief comment in the example clarifying that:
ontogglefires on all state changes (open/closed), requiring the state checkonclosefires only when closing (as shown in Onclose.svelte)Optional enhancement:
<Dropdown simple ontoggle={(ev) => { + // ontoggle fires on both open and close; check newState if (ev.newState === 'closed') { console.log('closed by ontoggle'); } }}>src/routes/docs/components/dropdown.md (1)
237-245: Add explanatory text to clarify onclose vs ontoggle usage.The new section demonstrates both event handlers but doesn't explain when to use each. Based on the example implementations, users might not understand:
onclose: Fires only when the dropdown closesontoggle: Fires on all state changes (requires checkingev.newState)Consider adding a brief explanation before the code examples:
### onclose and ontoggle + +Use `onclose` to run a callback when the dropdown closes. For more control over state changes, use `ontoggle` which fires on both open and close events. ```svelte example class="flex justify-center items-start h-44" hideResponsiveButtons
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/routes/docs-examples/components/dropdown/Onclose.svelte(1 hunks)src/routes/docs-examples/components/dropdown/Ontoggle.svelte(1 hunks)src/routes/docs/components/dropdown.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-18T09:38:03.879Z
Learnt from: Chizaruu
PR: themesberg/flowbite-svelte#1465
File: src/lib/forms/Timepicker.svelte:105-116
Timestamp: 2024-10-18T09:38:03.879Z
Learning: In `Timepicker.svelte`, the `Dropdown` opens as intended and is used exclusively with the 'dropdown' and 'timerange-dropdown' Timepicker types.
Applied to files:
src/routes/docs-examples/components/dropdown/Ontoggle.sveltesrc/routes/docs-examples/components/dropdown/Onclose.sveltesrc/routes/docs/components/dropdown.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Playwright and Lint
🔇 Additional comments (1)
src/routes/docs/components/dropdown.md (1)
233-233: LGTM!The height adjustment from
h-40toh-44maintains consistency with the new examples added below.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lib/dropdown/Dropdown.svelte(1 hunks)src/routes/docs-examples/components/toast/Events.svelte(1 hunks)src/routes/docs/components/toast.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/dropdown/Dropdown.svelte
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Playwright and Lint
🔇 Additional comments (1)
src/routes/docs-examples/components/toast/Events.svelte (1)
5-5: No changes needed: Toast supportsonclosevia restProps forwarding. The component dispatches acloseevent in its_closehandler and spreads unknown props onto the root<div>, soonclose={()=>…}will fire correctly.
|
@shinokada have you seen those Nitpick comments from coderabbit? |
|
@jjagielka Yes, gone through the coderabbit issues and updated accordingly. |
2 participants
Closes #1789
📑 Description
Add
oncloseprop toPopper.svelte.Status
✅ Checks
api-checkdirectory as requiredpnpm check && pnpm test:e2eSummary by CodeRabbit
New Features
Documentation