Repository navigation
Conversation
|
@jak-pan is attempting to deploy a commit to the LoopLabs Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Pull request overview
This PR adds Terms of Service and Privacy Policy functionality to the footer by implementing a modal-based approach instead of separate pages. The implementation includes a new reusable Modal component with animations and scroll behavior management.
Key changes:
- New Modal component with backdrop blur, animations, and keyboard navigation
- Terms of Service and Privacy Policy content components
- Legal component to manage modal states and trigger buttons
- Footer refactored to use modal-based legal links instead of route-based navigation
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/components/ui/modal/modal.tsx | New reusable modal component with animations, scroll lock, and escape key handling |
| src/components/footer/terms-of-service.tsx | Terms of Service content component with structured legal text |
| src/components/footer/privacy-policy.tsx | Privacy Policy content component detailing data handling practices |
| src/components/footer/legal.tsx | Component managing modal states and rendering legal link buttons |
| src/components/footer/footer.tsx | Updated to import and use the new Legal component instead of inline implementation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <motion.div | ||
| initial={{ opacity: 0, scale: 0.95, y: 20 }} | ||
| animate={{ opacity: 1, scale: 1, y: 0 }} | ||
| exit={{ opacity: 0, scale: 0.95, y: 20 }} | ||
| transition={{ duration: 0.2 }} | ||
| onClick={(e) => e.stopPropagation()} | ||
| onWheel={(e) => e.stopPropagation()} | ||
| className={twMerge( | ||
| "bg-white rounded-xl max-w-2xl w-full max-h-[90vh] overflow-hidden flex flex-col shadow-lg", | ||
| className | ||
| )} |
There was a problem hiding this comment.
The modal backdrop and container are missing accessibility attributes. The modal should have role="dialog", aria-modal="true", and aria-labelledby pointing to the title element. The backdrop should also have aria-hidden="true" to hide it from screen readers. These attributes are essential for proper screen reader support.
| <Heading size="medium" className="text-purple"> | ||
| {title} | ||
| </Heading> |
There was a problem hiding this comment.
The modal title should have an id attribute so it can be referenced by aria-labelledby on the modal container. This creates a proper association between the modal and its label for screen readers.
| useLayoutEffect(() => { | ||
| if (isOpen) { | ||
| // Prevent body scroll when modal is open | ||
| const originalOverflow = document.body.style.overflow; | ||
| document.body.style.overflow = "hidden"; | ||
|
|
||
| return () => { | ||
| // Restore body scroll when modal closes | ||
| document.body.style.overflow = originalOverflow; | ||
| }; | ||
| } | ||
| }, [isOpen]); |
There was a problem hiding this comment.
The modal is missing focus management. When the modal opens, focus should be trapped within the modal and moved to the first focusable element (typically the close button). When the modal closes, focus should be restored to the element that triggered the modal. This is critical for keyboard and screen reader users.
| <ul className="list-disc list-inside ml-4 space-y-2 text-purple font-geist font-normal text-base leading-tight"> | ||
| <li>IP addresses</li> | ||
| <li>Browser type and version</li> | ||
| <li>Operating system</li> | ||
| <li>Pages visited and time spent on pages</li> | ||
| <li>Referring website addresses</li> | ||
| </ul> | ||
| <Paragraph size="medium"> | ||
| This information is standard server log data that is automatically | ||
| collected by web servers and is not used to identify individual users. | ||
| </Paragraph> | ||
|
|
||
| <Paragraph size="medium"> | ||
| <strong>3. How We Use Information</strong> | ||
| </Paragraph> | ||
| <Paragraph size="medium"> | ||
| The automatically logged information is used solely for: | ||
| </Paragraph> | ||
| <ul className="list-disc list-inside ml-4 space-y-2 text-purple font-geist font-normal text-base leading-tight"> | ||
| <li>Ensuring the security and proper functioning of our website</li> | ||
| <li>Diagnosing technical issues and improving website performance</li> | ||
| <li>Analyzing general usage patterns (in aggregate, not individual)</li> | ||
| </ul> |
There was a problem hiding this comment.
[nitpick] The styling for the unordered lists is duplicated on lines 23 and 41. Consider extracting this to a reusable constant or component to improve maintainability: const listClassName = "list-disc list-inside ml-4 space-y-2 text-purple font-geist font-normal text-base leading-tight";
| useEffect(() => { | ||
| const handleEscape = (e: KeyboardEvent) => { | ||
| if (e.key === "Escape") { | ||
| onClose(); | ||
| } | ||
| }; | ||
| if (isOpen) { | ||
| window.addEventListener("keydown", handleEscape); | ||
| } | ||
| return () => { | ||
| window.removeEventListener("keydown", handleEscape); | ||
| }; | ||
| }, [isOpen, onClose]); |
There was a problem hiding this comment.
The onClose callback in the dependency array of the useEffect could cause the effect to re-run unnecessarily if the callback reference changes on every render. Consider wrapping onClose with useCallback in the parent component, or remove it from the dependency array and use a ref instead to avoid potential performance issues.
| <button | ||
| onClick={() => setPrivacyOpen(true)} | ||
| className="font-inter font-medium cursor-pointer" | ||
| > | ||
| <Paragraph size="small" className="text-purple-dim lg:text-purple"> | ||
| Privacy Policy | ||
| </Paragraph> | ||
| </button> | ||
| <button | ||
| onClick={() => setTosOpen(true)} | ||
| className="font-inter font-medium cursor-pointer" | ||
| > | ||
| <Paragraph size="small" className="text-purple-dim lg:text-purple"> | ||
| Terms of Service | ||
| </Paragraph> | ||
| </button> |
There was a problem hiding this comment.
The buttons should have type="button" to prevent them from accidentally submitting a form if this component is used within a form context. This is a best practice for non-submit buttons.
| <button | ||
| onClick={onClose} | ||
| className="text-purple hover:text-purple-dim transition-colors font-geist text-2xl leading-none" | ||
| aria-label="Close modal" | ||
| > | ||
| × | ||
| </button> |
There was a problem hiding this comment.
The close button should have type="button" to prevent it from accidentally submitting a form if the modal content contains a form. This is a best practice for non-submit buttons.
No description provided.