-
Notifications
You must be signed in to change notification settings - Fork 61
Added cookie consent #316
base: master
Are you sure you want to change the base?
Added cookie consent #316
Conversation
Codecov Report
@@ Coverage Diff @@
## master #316 +/- ##
==========================================
+ Coverage 50.63% 50.66% +0.02%
==========================================
Files 66 66
Lines 3772 3774 +2
Branches 444 444
==========================================
+ Hits 1910 1912 +2
Misses 1767 1767
Partials 95 95
Continue to review full report at Codecov.
|
aec0b3d
to
90375cb
Compare
@Shekharrajak Can you please review this again? |
@Kajol-Kumari , can you please provide some description how this PR will work and perform better ? Can you make sure you write details in PR itself ? Adding comments to your code change will help to approve and understand it better. |
Codecov Report
@@ Coverage Diff @@
## master #316 +/- ##
==========================================
+ Coverage 50.16% 50.19% +0.02%
==========================================
Files 66 66
Lines 3873 3875 +2
Branches 450 450
==========================================
+ Hits 1943 1945 +2
Misses 1836 1836
Partials 94 94
Continue to review full report at Codecov.
|
@Shekharrajak I have added the required description and comments.can u please review it now. |
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.
Overall it is looking good, but I am still not sure what are the browser cookies the site will be using and how it will behave in mobile/tablet device size.
src/app/app.module.ts
Outdated
// configuration of cookie consent | ||
const cookieConfig: NgcCookieConsentConfig = { | ||
cookie: { | ||
domain: 'localhost' // or 'your.domain.com' // it is mandatory to set a domain, for cookies to work properly (see https://goo.gl/S2Hy2A) |
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.
What will be the domain when it is in production or staging environment? At that time how will we configure it? Can it pick the domain without setting it - can you explore about it ?
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.
@Shekharrajak we just need to change the domain in here for the production mode and in it's doc, i couldn't find any feature of automatic domain detection.So will have to change it manually at the time of production.
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.
Try to make it environment variable then.
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.
Thank you for suggestion @Shekharrajak . I have made the suggested changes. Can you please now check if this looks good to you?
Feature added:
The cookie consent will appear when-
someone opens the site for the first time
everytime when someone opens the site in incognito window
Screenshot:
For tablets:
Mobile view: