-
Notifications
You must be signed in to change notification settings - Fork 2.4k
feat: improve cookie banner visibility and engagement #8903
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Review SummaryI've reviewed the changes in this PR and identified the following issues that should be addressed:
|
| /> | ||
| <ReactCookieConsent | ||
| location="bottom" | ||
| buttonText="Accept Cookies" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Internationalization: Consider wrapping all user-facing strings (e.g., 'Accept Cookies', 'Decline', and the banner message) in your i18n/translation function instead of hardcoding them. This ensures consistency and easier localization across Roo Code.
This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.
|
🚀 Preview deployed! Your changes have been deployed to Vercel: Preview URL: https://roo-code-website-m3qap6487-roo-code.vercel.app This preview will be updated automatically when you push new commits to this PR. |
| } | ||
|
|
||
| // Check if consent cookie exists | ||
| const cookieExists = document.cookie.includes(CONSENT_COOKIE_NAME) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This cookie check using string includes is fragile and could produce false positives if another cookie's value contains 'roo-code-cookie-consent' as a substring. The codebase already imports getCookieConsentValue from react-cookie-consent in the consent-manager module. Consider using that function or implementing proper cookie parsing to check for the exact cookie name.
| @keyframes pulse { | ||
| 0%, 100% { | ||
| box-shadow: 0 0 0 0 rgba(34, 197, 94, 0.4); | ||
| } | ||
| 50% { | ||
| box-shadow: 0 0 0 8px rgba(34, 197, 94, 0); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The global pulse keyframe definition will override Tailwind's built-in animate-pulse utility, potentially breaking any existing usage of that class across the application. Consider renaming this to something more specific like cookiePulse to avoid conflicts with Tailwind's animation system.
|
There seem to be some problems with this PR, the banner is appearing 2 times when clicking "Accept", I'll be closing it for now but let me know if it should be reopened. |
Enhances the cookie consent banner to increase user interaction rates while maintaining a tasteful, non-intrusive experience.
Changes
Visual Enhancements
User Experience
Technical Implementation
dangerouslySetInnerHTML(safe - only static CSS, no user input)Testing
Notes
dangerouslySetInnerHTMLusage is safe in this context as it only contains static CSS keyframe definitions, not user-generated contentImpact
Expected to significantly improve cookie consent interaction rates while maintaining GDPR compliance and a professional user experience.
Important
Enhances cookie consent banner with visual, UX improvements, and state management using React hooks in
CookieConsentWrapper.CookieConsentWrapper.CookieConsentWrapper.dangerouslySetInnerHTMLfor animations.This description was created by
for 5fd88f7. You can customize this summary. It will automatically update as commits are pushed.