-
Notifications
You must be signed in to change notification settings - Fork 18
review: cookie consent banner #65
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
|
It looks great. |
|
shall i close #64 |
sonegs
left a comment
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.
@0010aor LGTM 🚀
| const CONSENT_KEY = 'analytics_consent' | ||
|
|
||
| const AnalyticsConsent: FC = () => { | ||
| const [, setAcceptConsent] = useState<boolean>(false) |
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.
🚔 🚔 🚔
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.
const [acceptConsent, setAcceptConsent] = useState<boolean>(false)
But maybe you must to check the use of this state (looks like it is not neccessary)
| setAcceptConsent(false) | ||
| setShowConsent(true) | ||
| } | ||
| }, []) |
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.
If we not use some dependency in useEffect hook, the performance will be damaged because this code will be run with each javascript change.
Checking the logic inside of this useEffect, looks like we just need to check the changes in storedConsent const, so we can add it like a dependency.
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.
| }, []) | |
| }, [storedConsent]) |
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.
If we not use some dependency in useEffect hook, the performance will be damaged because this code will be run with each javascript change.
Checking the logic inside of this useEffect, looks like we just need to check the changes in storedConsent const, so we can add it like a dependency.
I think in this case, the empty dependency array ([]) is intentional we just want the useEffect to run once when the component mounts. Since we’re reading from localStorage on initial load, we only need to check the user’s consent status once when the app starts.
| import { useTranslation } from 'react-i18next' | ||
| import { FaTimes as CloseIcon } from 'react-icons/fa' | ||
|
|
||
| const CONSENT_KEY = 'analytics_consent' |
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.
Could be interesting add a directory to save this kind of const values. Add a new directory in frontend/src/ called lib (in the future, we will a refactor of the arquitecture about that). Inside it, add a new directory called const.
In this directory, we can add a new file called analytics.ts, where we can save and export this kind of consts. Please, add a comment to explain the purpose of this const.
This is a proposed revision of #64 (original PR by @muhammedsirajudeen).
I've tested and adjusted some things. Happy to discuss or merge if it looks good.
Thanks a lot for the contribution! Here are some things that I think we can improve:
@/theme.tsxfor consistent UI, or creates new semantic tokens.main.tsxas the entry point.Please don’t forget to use Biome to format the code and set up the pre-commit tool to resolve linting issues before committing.
Great work!