Skip to content
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

Add possibility configure path cookie #18

Merged
merged 1 commit into from Mar 2, 2017

Conversation

5 participants
@anijra
Copy link
Contributor

commented Feb 27, 2017

No description provided.

@SylvainGuittard SylvainGuittard requested a review from clash82 Feb 27, 2017

@clash82

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2017

@anijra could you please revert files permission change?

@clash82
Copy link
Contributor

left a comment

Good work, some small improvements submitted

@@ -25,6 +25,9 @@ public function getConfigTreeBuilder()
->defaultValue('365')
->info('how many days banner should be hidden when user accepts policy?')
->end()
->integerNode('path')

This comment has been minimized.

Copy link
@clash82

clash82 Feb 27, 2017

Contributor

AFAIK this is a scalarNode

would be great if you could also provide some details using info key

README.md Outdated
@@ -92,6 +92,7 @@ Parameter | Default value | Description
---------------- | ---------------------------------------------- | -----------
cookieName | privacyCookieAccepted | Sets your own status cookie name
cookieValidity | 365 | Says how many days privacy banner should be hidden when user accepts policy?
cookiePath | null | Specifies the cookie path

This comment has been minimized.

Copy link
@clash82

clash82 Feb 27, 2017

Contributor

We could add some context here:

Specifies the cookie path (by default cookie will be available only for the current domain)

@@ -3,6 +3,8 @@ parameters:
ez_privacy_cookie.days: 365
# name to be used to store cookie status
ez_privacy_cookie.cookie_name: privacyCookieAccepted
# Specifies the cookie path

This comment has been minimized.

Copy link
@clash82

clash82 Feb 27, 2017

Contributor

please, use lowercase to be consistent with rest of the comments here ;)

document.cookie = name + '=' + value + '; ' + expires;

var cookiePath = '';
if(path) {

This comment has been minimized.

Copy link
@clash82

@clash82 clash82 requested a review from SylvainGuittard Feb 27, 2017

@clash82 clash82 requested a review from damianz5 Feb 27, 2017

@andrerom

This comment has been minimized.

Copy link
Member

commented Feb 28, 2017

Still some chmod changes here!

@clash82

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2017

@anijra two files are still waiting for permissions change

@SylvainGuittard
Copy link

left a comment

Thank you @anijra for the contribution

@SylvainGuittard

This comment has been minimized.

Copy link

commented Mar 2, 2017

@anijra could you please rebase your PR in order to have just one commit. I will merge it after.

@anijra anijra force-pushed the anijra:master branch from 0fca175 to baad86c Mar 2, 2017

@SylvainGuittard SylvainGuittard merged commit c7c5567 into ezsystems:master Mar 2, 2017

1 check passed

ezrobot Code review by ezrobot
Details
@clash82

This comment has been minimized.

Copy link
Contributor

commented Mar 2, 2017

@SylvainGuittard please, don't forget to create a net version tag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.