Cookie_Hover_Box #1127

bhaggya · 2022-01-24T06:13:31Z

mealthebear · 2022-01-25T02:42:50Z

The PopUp.js file may be better suited for the components folder

mealthebear

Found some areas for touch ups; this looks pretty good so far!

bhaggya · 2022-01-27T02:31:08Z

src/pages/Privacy/PopUp.js

+  const setCookie = function (cookieEmail, cookieValue, expDays) {
+    date.setTime(date.getTime() + expDays);
+
+    const expires = 'expires=' + date.toUTCString();
+
+    document.cookie = cookieEmail + cookieValue + ';' + expires + ';path=/';
+
+    return document.cookie;
+  };
+
+  const deleteCookie = function (cookieEmail) {
+    date.setTime(date.getTime());
+    const expires = 'expires=' + date.toUTCString();
+    document.cookie = cookieEmail + expires + ';path=/';
+  };
+  const acceptCookieConsent = () => {
+    deleteCookie('civictecindex_cookie_consent');
+    setCookie('civictecindex_cookie_consent', 1, 30);


So, I'm not sure what the intended behavior for the cookie should be: currently the way it looks like it's working is the date is registered upon the user visiting the site, not when the user clicks 'Accept' on the cookie pop-up.

One concern I wanted to bring up is in certain scenarios where the user takes a while to select 'Accept', or visits the page but leaves it open for a long period of time.

This could potentially paint an inaccurate picture of traffic. Then again, maybe we only want to know the time when they just visit the site.

Wanted to bring this up to attention after playing around with and testing the cookie in the browser. :)

Hi Dennis
I was not told anything about functionality, I was just told to code as per reference with 311 data. I think the functionality will be told after

mealthebear · 2022-01-25T23:41:10Z

src/pages/Privacy/PopUp.js

+    deleteCookie('civictecindex_cookie_consent');
+    setCookie('civictecindex_cookie_consent', 1, 30);


Add missing letter 'h' to 'civictechindex'

Suggested change

deleteCookie('civictecindex_cookie_consent');

setCookie('civictecindex_cookie_consent', 1, 30);

deleteCookie('civictechindex_cookie_consent');

setCookie('civictechindex_cookie_consent', 1, 30);

mealthebear · 2022-01-25T23:47:34Z

src/pages/Privacy/PopUp.js

+    date.setTime(date.getTime() + expDays);
+
+    const expires = 'expires=' + date.toUTCString();
+
+    document.cookie = cookieEmail + cookieValue + ';' + expires + ';path=/';
+
+    return document.cookie;
+  };
+
+  const deleteCookie = function (cookieEmail) {
+    date.setTime(date.getTime());
+    const expires = 'expires=' + date.toUTCString();
+    document.cookie = cookieEmail + expires + ';path=/';


I recommend replacing string concatenation with template literals.

Suggested change

date.setTime(date.getTime() + expDays);

const expires = 'expires=' + date.toUTCString();

document.cookie = cookieEmail + cookieValue + ';' + expires + ';path=/';

return document.cookie;

};

const deleteCookie = function (cookieEmail) {

date.setTime(date.getTime());

const expires = 'expires=' + date.toUTCString();

document.cookie = cookieEmail + expires + ';path=/';

date.setTime(date.getTime() + expDays);

const expires = `expires=${date.toUTCString()}`;

document.cookie = `${cookieEmail}${cookieValue};${expires};path=/`;

return document.cookie;

};

const deleteCookie = function (cookieEmail) {

date.setTime(date.getTime());

const expires = `expires=${date.toUTCString()}`;

document.cookie = `${cookieEmail}${expires};path=/`

mealthebear · 2022-01-25T23:57:30Z

So, I'm not sure what the intended behavior for the cookie should be: currently the way it looks like it's working is the date is registered upon the user visiting the site, not when the user clicks 'Accept' on the cookie pop-up.

One concern I wanted to bring up is in certain scenarios where the user takes a while to select 'Accept', or visits the page but leaves it open for a long period of time.

This could potentially paint an inaccurate picture of traffic. Then again, maybe we only want to know the time when they just visit the site.

Wanted to bring this up to attention after playing around with and testing the cookie in the browser. :)

bruceplai · 2022-01-26T00:22:27Z

Just a reminder to close the privacy popup during the footer.spec test. Also, if possible, please create tests in a privacy.spec.js test file so that we have coverage of this new functionality. If any help/guidance is needed, let me know. Thank you!

bhaggya · 2022-01-27T02:31:08Z

src/pages/Privacy/PopUp.js

+  const setCookie = function (cookieEmail, cookieValue, expDays) {
+    date.setTime(date.getTime() + expDays);
+
+    const expires = 'expires=' + date.toUTCString();
+
+    document.cookie = cookieEmail + cookieValue + ';' + expires + ';path=/';
+
+    return document.cookie;
+  };
+
+  const deleteCookie = function (cookieEmail) {
+    date.setTime(date.getTime());
+    const expires = 'expires=' + date.toUTCString();
+    document.cookie = cookieEmail + expires + ';path=/';
+  };
+  const acceptCookieConsent = () => {
+    deleteCookie('civictecindex_cookie_consent');
+    setCookie('civictecindex_cookie_consent', 1, 30);


So, I'm not sure what the intended behavior for the cookie should be: currently the way it looks like it's working is the date is registered upon the user visiting the site, not when the user clicks 'Accept' on the cookie pop-up.

One concern I wanted to bring up is in certain scenarios where the user takes a while to select 'Accept', or visits the page but leaves it open for a long period of time.

This could potentially paint an inaccurate picture of traffic. Then again, maybe we only want to know the time when they just visit the site.

Wanted to bring this up to attention after playing around with and testing the cookie in the browser. :)

Hi Dennis
I was not told anything about functionality, I was just told to code as per reference with 311 data. I think the functionality will be told after

bhaggya · 2022-01-27T02:37:56Z

So, I'm not sure what the intended behavior for the cookie should be: currently the way it looks like it's working is the date is registered upon the user visiting the site, not when the user clicks 'Accept' on the cookie pop-up.

One concern I wanted to bring up is in certain scenarios where the user takes a while to select 'Accept', or visits the page but leaves it open for a long period of time.

This could potentially paint an inaccurate picture of traffic. Then again, maybe we only want to know the time when they just visit the site.

Wanted to bring this up to attention after playing around with and testing the cookie in the browser. :)

I don;t understand what you are saying

mealthebear · 2022-01-27T19:35:36Z

My bad; ignore my previous comment. I got confused if we had already talked about how the cookie works.

mealthebear · 2022-01-28T01:27:28Z

src/pages/Privacy/PopUp.js

+    date.setTime(date.getTime() + expDays);
+    const expires = 'expires=' + date.toUTCString();
+
+    document.cookie = `${cookieEmail} + ${cookieValue} + ';' + ${expires} + ';path=/'`;


With template literals, the pluses don't need to be used. It will actually show up in the string that way.

Suggested change

document.cookie = `${cookieEmail} + ${cookieValue} + ';' + ${expires} + ';path=/'`;

document.cookie = `${cookieEmail}${cookieValue}';'${expires}';path=/'`;

mealthebear · 2022-01-28T01:28:04Z

src/pages/Privacy/PopUp.js

+  const deleteCookie = function (cookieEmail) {
+    date.setTime(date.getTime());
+    const expires = 'expires=' + date.toUTCString();
+    document.cookie = `${cookieEmail} + ${expires} + ';path=/'`;


Same thing here with template literal

Suggested change

document.cookie = `${cookieEmail} + ${expires} + ';path=/'`;

document.cookie = `${cookieEmail}${expires}';path=/'`;

mealthebear

Looks good to me. Thanks Bhaggya!

bhaggya · 2022-01-28T02:13:45Z

Looks good to me. Thanks Bhaggya!

Thanks for reviewing my code and letting me know

bruceplai · 2022-01-29T04:34:29Z

@smsada @kevindphan there is still the visual side effect of the privacy popup shifting to the left a few pixels when hovering over the header menu items (possibly due the scroll bar disappearing/appearing). Can we still merge this PR?

bruceplai · 2022-02-01T04:12:20Z

PMs have decided to hold off on merging this until after MVP

codeclimate · 2022-02-25T01:11:55Z

Code Climate has analyzed commit 4f54f78 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 81.0% (0.2% change).

View more on Code Climate.

bhaggya requested review from maxskewes, bruceplai and mealthebear January 24, 2022 06:13

mealthebear requested changes Jan 25, 2022

View reviewed changes

bhaggya commented Jan 27, 2022

View reviewed changes

mealthebear reviewed Jan 28, 2022

View reviewed changes

mealthebear previously approved these changes Jan 28, 2022

View reviewed changes

bruceplai dismissed mealthebear’s stale review via 53f58af January 29, 2022 19:33

smsada mentioned this pull request Jan 30, 2022

CTI: PM/Org Meeting Agenda #856

Open

bruceplai marked this pull request as draft February 1, 2022 04:20

bhaggya and others added 4 commits February 1, 2022 11:52

Cookie_Hover_Box

e55a67e

Added test file and changes made as per review

4ea44aa

Changes made to use template literals

530e07a

Update cypress tests for privacy popup

bc57c3c

bruceplai force-pushed the Hover_Cookie_Box branch from 2e372fc to bc57c3c Compare February 1, 2022 19:52

bruceplai and others added 6 commits February 1, 2022 12:18

Merge branch 'main' into Hover_Cookie_Box

7e52913

Merge branch 'main' into Hover_Cookie_Box

501edc3

Merge branch 'main' into Hover_Cookie_Box

46c671a

Merge branch 'main' into Hover_Cookie_Box

b1d2cee

Merge branch 'main' into Hover_Cookie_Box

0b72f53

Fixed scrollbar_issue

9510113

bhaggya marked this pull request as ready for review February 18, 2022 02:51

Merge branch 'main' into Hover_Cookie_Box

4f54f78

bruceplai approved these changes Feb 25, 2022

View reviewed changes

bruceplai merged commit 47017c1 into main Feb 25, 2022

bruceplai deleted the Hover_Cookie_Box branch February 25, 2022 02:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cookie_Hover_Box #1127

Cookie_Hover_Box #1127

bhaggya commented Jan 24, 2022

mealthebear commented Jan 25, 2022

mealthebear left a comment

bhaggya Jan 27, 2022

mealthebear Jan 25, 2022

mealthebear Jan 25, 2022

mealthebear commented Jan 25, 2022

bruceplai commented Jan 26, 2022

bhaggya Jan 27, 2022

bhaggya commented Jan 27, 2022

mealthebear commented Jan 27, 2022

mealthebear Jan 28, 2022

mealthebear Jan 28, 2022

mealthebear left a comment

bhaggya commented Jan 28, 2022

bruceplai commented Jan 29, 2022

bruceplai commented Feb 1, 2022

codeclimate bot commented Feb 25, 2022

		deleteCookie('civictecindex_cookie_consent');
		setCookie('civictecindex_cookie_consent', 1, 30);

	document.cookie = `${cookieEmail} + ${cookieValue} + ';' + ${expires} + ';path=/'`;
	document.cookie = `${cookieEmail}${cookieValue}';'${expires}';path=/'`;

	document.cookie = `${cookieEmail} + ${expires} + ';path=/'`;
	document.cookie = `${cookieEmail}${expires}';path=/'`;

Cookie_Hover_Box #1127

Cookie_Hover_Box #1127

Conversation

bhaggya commented Jan 24, 2022

mealthebear commented Jan 25, 2022

mealthebear left a comment

Choose a reason for hiding this comment

bhaggya Jan 27, 2022

Choose a reason for hiding this comment

mealthebear Jan 25, 2022

Choose a reason for hiding this comment

mealthebear Jan 25, 2022

Choose a reason for hiding this comment

mealthebear commented Jan 25, 2022

bruceplai commented Jan 26, 2022

bhaggya Jan 27, 2022

Choose a reason for hiding this comment

bhaggya commented Jan 27, 2022

mealthebear commented Jan 27, 2022

mealthebear Jan 28, 2022

Choose a reason for hiding this comment

mealthebear Jan 28, 2022

Choose a reason for hiding this comment

mealthebear left a comment

Choose a reason for hiding this comment

bhaggya commented Jan 28, 2022

bruceplai commented Jan 29, 2022

bruceplai commented Feb 1, 2022

codeclimate bot commented Feb 25, 2022