fix: add allowClickOutside prop to Modal for improved interaction #735
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
📝 WalkthroughWalkthroughThis PR centralizes disclaimer handling across platforms: reads/stores acceptance in localStorage, prevents logout on outside-click by triggering a pulse animation and hint, renders disclaimers until accepted, and adds dialog/modal API opts for controlling outside-click and close-button behavior. Changes
Sequence DiagramsequenceDiagram
participant User
participant App
participant localStorage
participant Modal
rect rgba(220,240,255,0.5)
User->>App: Navigate to protected route / Mount
App->>localStorage: safeGetItem(DISCLAIMER_KEY)
localStorage-->>App: value or null
end
alt accepted
App->>User: Render protected content
else not accepted
App->>Modal: Render disclaimer modal (open)
User->>Modal: Interact
alt Click "I Understand"
Modal->>localStorage: safeSetItem(DISCLAIMER_KEY, "true")
localStorage-->>App: persisted
App->>User: Render protected content
else Click outside
Modal->>App: invoke onClickOutside / handler
App->>App: set isPulsing = true, showHint = true
App->>User: Play pulse animation and show hint tooltip
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
… styling and interaction
…improved user interaction
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@platforms/blabsy/src/components/layout/common-layout.tsx`:
- Around line 59-96: The mailto anchor currently uses the className
'outline-none' which removes keyboard focus indicators; update the anchor in
common-layout.tsx (the <a href='mailto:...'> element) to add focus-visible
utility classes consistent with other patterns (e.g., use focus-visible:ring-2,
focus-visible:ring-yellow-400, focus-visible:ring-offset-2 and keep outline-none
only for non-focus visuals) so keyboard users see a clear focus ring; ensure the
class list mirrors the project's .main-tab/.custom-underline focus-visible
approach and preserves existing styling while restoring accessible focus
outlines.
- Line 5: The handleOutsideClick handler currently sets a timeout that calls
setIsPulsing(false) which may fire after the component unmounts; add a ref
(e.g., pulseTimeoutRef) to store the timeout ID when you call setTimeout, clear
that timeout in the CommonLayout component's cleanup (useEffect return) and also
clear it before setting a new timeout inside handleOutsideClick so you never
leave an untracked timer running; update any checks around
isPulsing/setIsPulsing to avoid calling state after unmount.
- Around line 14-27: The component initializes disclaimerAccepted to true which
lets the app render before localStorage is read and also calls
localStorage.getItem/setItem without guards; change ProtectedLayout to track a
separate "storageChecked" flag (e.g., initialize disclaimerAccepted to false or
undefined and add storageChecked state) and only render children after
storageChecked is true (or render a lightweight placeholder), and wrap all
localStorage access in try-catch blocks around the calls referenced by
DISCLAIMER_KEY, the useEffect that calls localStorage.getItem, and wherever
localStorage.setItem is used so failures in private mode/quota errors are
handled (fall back to non-persistent acceptance and still call
setDisclaimerAccepted/set storageChecked inside the try/catch).
In `@platforms/eVoting/src/app/`(app)/layout.tsx:
- Around line 44-49: LocalStorage access in the useEffect that reads
DISCLAIMER_KEY can throw in restricted/private browsing; wrap the access in a
safe guard (try/catch or a helper like safeStorageGet/safeStorageSet) around
localStorage.getItem and localStorage.setItem so runtime errors are swallowed
and a sensible fallback (false) is used, and update both the useEffect that
reads DISCLAIMER_KEY (where setDisclaimerAccepted is called) and the code path
that writes the key (the places that call localStorage.setItem for
DISCLAIMER_KEY) to use this safe helper; reference the useEffect,
DISCLAIMER_KEY, and setDisclaimerAccepted symbols when making the change.
- Around line 98-105: The pulse animation isn't applied because there's no
.animate-pulse-scale class and the inline style only sets animationName; update
the component rendering logic (the className and style that reference isPulsing,
animate-pulse-scale and animationName) to use a real animation
declaration—either add a CSS class .animate-pulse-scale that sets
animation-duration, animation-timing-function and animation-iteration-count for
the `@keyframes` pulse-scale, or replace the conditional class with a Tailwind
arbitrary animation (e.g. use isPulsing &&
"animate-[pulse-scale_1.2s_linear_infinite]") and remove the inline
style.animationName so the animation runs.
In `@platforms/group-charter-manager/src/components/disclaimer-modal.tsx`:
- Around line 28-32: Wrap all direct localStorage accesses in try/catch and
guard with a feature check before use: in the useEffect that reads
DISCLAIMER_KEY and calls setDisclaimerAccepted, catch exceptions from
localStorage.getItem and default to false; similarly, guard the code paths that
write DISCLAIMER_KEY (the accept/close handlers around the modal, referenced at
lines handling save of DISCLAIMER_KEY) so they try/catch localStorage.setItem
and degrade gracefully (update state without persisting if storage is
unavailable). Reference the existing useEffect, DISCLAIMER_KEY,
setDisclaimerAccepted, and the modal accept/close handlers to locate where to
add these guards.
In `@platforms/pictique/src/lib/ui/Modal/Modal.svelte`:
- Around line 23-42: The handler currently calls onclose even when the modal is
kept open (branch where !isInDialog && !allowClickOutside); remove the onclose
invocation from that branch so onclose is only called when modal.close() is
invoked, and if you need a signal for the blocked outside click add and call a
separate optional callback (e.g., onOutsideClick?) instead; update the code
paths around isInDialog, allowClickOutside, modal.close(), modal.animate() and
onclose to ensure onclose is never invoked when the animate/pulse branch runs.
🧹 Nitpick comments (3)
platforms/group-charter-manager/src/components/disclaimer-modal.tsx (1)
34-39: Clear the pulse timeout on unmount to avoid stale state updates.The timeout can fire after unmount and trigger a setState warning.
♻️ Suggested cleanup
-import { useState, useEffect } from "react"; +import { useState, useEffect, useRef } from "react"; @@ const [showHint, setShowHint] = useState(false); const [isPulsing, setIsPulsing] = useState(false); + const pulseTimeoutRef = useRef<number | null>(null); @@ const handleInteractOutside = (e: Event) => { e.preventDefault(); setIsPulsing(true); setShowHint(true); - setTimeout(() => setIsPulsing(false), 400); + if (pulseTimeoutRef.current) window.clearTimeout(pulseTimeoutRef.current); + pulseTimeoutRef.current = window.setTimeout(() => setIsPulsing(false), 400); }; + + useEffect(() => { + return () => { + if (pulseTimeoutRef.current) window.clearTimeout(pulseTimeoutRef.current); + }; + }, []);platforms/eVoting/src/app/(app)/layout.tsx (1)
51-56: Clear the pulse timeout on unmount to avoid stale state updates.The timeout can fire after unmount and trigger a setState warning.
♻️ Suggested cleanup
-import { useEffect, useState } from "react"; +import { useEffect, useState, useRef } from "react"; @@ const [disclaimerAccepted, setDisclaimerAccepted] = useState(false); const [isPulsing, setIsPulsing] = useState(false); const [showHint, setShowHint] = useState(false); + const pulseTimeoutRef = useRef<number | null>(null); @@ const handleInteractOutside = (e: Event) => { e.preventDefault(); setIsPulsing(true); setShowHint(true); - setTimeout(() => setIsPulsing(false), 400); + if (pulseTimeoutRef.current) window.clearTimeout(pulseTimeoutRef.current); + pulseTimeoutRef.current = window.setTimeout(() => setIsPulsing(false), 400); }; + + useEffect(() => { + return () => { + if (pulseTimeoutRef.current) window.clearTimeout(pulseTimeoutRef.current); + }; + }, []);platforms/blabsy/src/components/layout/common-layout.tsx (1)
47-58: Consider moving inline keyframes to a stylesheetInjecting a
<style>tag on render can duplicate styles and complicate CSP. Prefer a CSS module/global stylesheet forpulse-scale.♻️ Suggested change
- <style>{` - `@keyframes` pulse-scale { - 0% { transform: scale(1); } - 25% { transform: scale(1.01); } - 50% { transform: scale(0.99); } - 75% { transform: scale(1.005); } - 100% { transform: scale(1); } - } - .animate-pulse-scale { - animation: pulse-scale 0.4s ease-in-out; - } - `}</style>Add the keyframes to your component stylesheet (module or global), e.g.:
`@keyframes` pulse-scale { 0% { transform: scale(1); } 25% { transform: scale(1.01); } 50% { transform: scale(0.99); } 75% { transform: scale(1.005); } 100% { transform: scale(1); } } .animate-pulse-scale { animation: pulse-scale 0.4s ease-in-out; }
…ng across components
…d user interaction
coodos
approved these changes
Jan 28, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@platforms/group-charter-manager/src/components/disclaimer-modal.tsx`:
- Around line 51-56: handleInteractOutside sets showHint true but never resets
it; add an auto-dismiss by scheduling setShowHint(false) (e.g., 2–4s) inside
handleInteractOutside, store the timer id in a ref (e.g., hintTimeoutRef) and
clear any existing timeout before creating a new one, and also clear the
timeout(s) in a useEffect cleanup to avoid leaks; keep the existing setIsPulsing
behavior but similarly clear its timeout when appropriate.
🧹 Nitpick comments (2)
platforms/pictique/src/routes/(protected)/+layout.svelte (1)
139-145: Consider adding accessibility attributes to the dynamic hint.The hint message appears dynamically but won't be announced to screen reader users. Adding
role="alert"ensures assistive technologies announce this important guidance.♿ Suggested accessibility improvement
{`#if` showHint} <p - class="mt-2 rounded-md border border-red-300 bg-red-100 px-3 py-2 text-center text-xs text-red-800" + class="mt-2 rounded-md border border-red-300 bg-red-100 px-3 py-2 text-center text-xs text-red-800" + role="alert" > 💡 You must accept the disclaimer to continue. This will only appear once. </p> {/if}platforms/group-charter-manager/src/components/disclaimer-modal.tsx (1)
63-81: Simplify the animation implementation.The animation is currently defined via three overlapping mechanisms:
- Tailwind arbitrary class:
animate-[pulse-scale_0.4s_ease-in-out]- Inline style:
animationName: isPulsing ? 'pulse-scale' : 'none'- styled-jsx keyframes
This is redundant and potentially fragile—styled-jsx scopes CSS, which may conflict with Tailwind's expectation of globally-accessible keyframes. Consider using just one approach.
♻️ Suggested simplification using inline style only
<DialogContent className={cn( - "max-w-lg mx-auto backdrop-blur-md p-6 rounded-lg", - isPulsing && "animate-[pulse-scale_0.4s_ease-in-out]" + "max-w-lg mx-auto backdrop-blur-md p-6 rounded-lg" )} style={{ - animationName: isPulsing ? 'pulse-scale' : 'none' + animation: isPulsing ? 'pulse-scale 0.4s ease-in-out' : 'none' }} hideCloseButton={true} onInteractOutside={handleInteractOutside} >
coodos
pushed a commit
that referenced
this pull request
Jan 30, 2026
* fix: add allowClickOutside prop to Modal for improved interaction * fix: enhance DisclaimerModal with tooltip and animation effects * fix: update ProtectedLayout to include disclaimer modal with enhanced styling and interaction * fix: enhance disclaimer modal with pulsing animation and tooltip for improved user interaction * fix: improve disclaimer handling with localStorage check and user hint * fix: update Modal to use onClickOutside prop for improved interaction and animation * fix: implement safe localStorage access methods for disclaimer handling across components * fix: improve disclaimer handling with enhanced localStorage access and user interaction
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of change
Disclaimer Modal Improvements
Added interactive disclaimer modals across platforms with:
Changes:
Issue Number
Closes #628
Type of change
How the change has been tested
Tested with group-charter-manager, eVoting and pictique, but couldn't test with Blabsy
Change checklist
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.