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

Now data-expires="" works as expected #72

Closed
wants to merge 1 commit into from

Conversation

webaddicto
Copy link
Contributor

Seems that data-expires="90" or data-expires=90 is always identified as string so here is the fix:

If this.options.expires is a string but is a valid number:

data-expires="90"

It calculates today + 90 days and writes the expiration in cookie-friendly format.

And if it is a string (but not a number):

data-expires="Thursday, August 28, 2018, 1:52:31 AM"

It is written as is.

Tested and works fine like this:

data-expires="Thursday, August 28, 2018, 1:52:31 AM"
or
data-expires="90"

Seems that data-expires="90" or data-expires=90 is always identified as string so here is the fix:

If this.options.expires is a string but is a valid number:

data-expires="90"

It calculates today + 90 days and writes the expiration in cookie-friendly format.

And if it is a string (but not a number):

data-expires="Thursday, August 28, 2018, 1:52:31 AM"

It is written as is.

Tested and works fine like this:

data-expires="Thursday, August 28, 2018, 1:52:31 AM"
or
data-expires="90"
@webaddicto webaddicto mentioned this pull request May 25, 2018
webaddicto added a commit to webaddicto/cookie-banner that referenced this pull request May 25, 2018
@zytzagoo
Copy link
Collaborator

I love the enthusiasm here, but I'm not sure defaulting to days as a unit (instead of seconds) is the right move here... Because, it's less inclusive (if nothing else).

Seconds work for every use case and everyone can calculate his duration in seconds one way or another. With days as the default unit (and with no way to indicate which unit one means, and no, we're not going down that road, because it will only complicate things more), you cannot specify less than 1 day for duration (which just feels wrong).

I was personally thinking of implementing it inside init(), at the same place this.options.expires is checked for some other types, and do an extra check to see if it's a number/numeric string and if so, convert it to an integer/number (which would leave existing code that sets cookie values/expiry as is).

What do you think about it?

@zytzagoo
Copy link
Collaborator

Closing since it's handled slightly differently via c3ecfc9 -- thanks for digging into it yourself and helping us resolve it!

@zytzagoo zytzagoo closed this May 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants