Skip to content
This repository has been archived by the owner on Nov 10, 2023. It is now read-only.

Feature #189: Cookie banner #296

Merged
merged 30 commits into from
Dec 4, 2019
Merged

Feature #189: Cookie banner #296

merged 30 commits into from
Dec 4, 2019

Conversation

Agupane
Copy link
Contributor

@Agupane Agupane commented Nov 25, 2019

Closes #189

Description

  • Implements cookie banner
  • Also, we were having problems with flow's warning and error, so we updated flow-types dependencys

Looks like this now:

image
image

@Agupane Agupane self-assigned this Nov 25, 2019
@Agupane Agupane changed the base branch from master to development November 25, 2019 13:16
@ghost
Copy link

ghost commented Nov 25, 2019

Travis automatic deployment:
https://pr296--safereact.review.gnosisdev.com

Storybook book automatic deployment:
https://pr296--safereactstorybook.review.gnosisdev.com

@Agupane Agupane requested a review from mmv08 November 25, 2019 13:35
@Agupane Agupane marked this pull request as ready for review November 25, 2019 13:35
@lukasschor
Copy link
Member

Looks good.

  1. Pressing on "Accept cookies" would be counted as a consent to "necessary" AND "analytics", even if "analytics" is not selected? Only closing the banner through the "x" would limit the consent to "necessary", right?
  2. Could we have the checkmark for "necessary" locked in, as shown in the designs? So it's not actually clickable. Currently it's possible to de-select necessary and close the banner, which should not be possible.
  3. I'll send you the link to the Cookie Policy as soon as possible (waiting for our DPO to approve it)

@Agupane
Copy link
Contributor Author

Agupane commented Nov 25, 2019

Hi @lukasschor !

  1. No, if the user did not selected analytics, only necessary would be counted as a consent. If you close it through "X", yes, it will count only necessary as consent, event if the analytics were selected.
  2. Of course, will do it
  3. Sounds good thanks!

@mmv08
Copy link
Member

mmv08 commented Nov 25, 2019

Also, we were having problems with flow's warning and error, so we updated flow-types dependencys

How did you do that? I thought it was not fixable 😄

Copy link
Member

@mmv08 mmv08 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cookiesSelector is used only in 1 component CookieBanner. I think we don't really need to put the state in redux store in that case.

src/index.js Show resolved Hide resolved
@Agupane
Copy link
Contributor Author

Agupane commented Nov 25, 2019

Hi @mikheevm !.

For updating the flow dependencies we did this:

- Deleted flow-typed folder
- flow-typed install
- Updated / fixed flowconfig file
- Updated / fixed eslintignore file

Also, about the cookiesSelector, I think it's a good idea to have it on the store, because later on we will need it for this two issues too:

@mmv08
Copy link
Member

mmv08 commented Nov 25, 2019

Sorry, I don't understand why we'd need it. Could you explain please? I thought we'd just load GA/Intercom script inside the cookiebanner component and then we can check localstorage in index.js

@Agupane
Copy link
Contributor Author

Agupane commented Nov 25, 2019

I prefer to not load those scripts inside the cookie banner to avoid mixing things, I would prefer to do it that separately. And do things like this PR. If you still don't like the idea I can change it

@gabitoesmiapodo
Copy link
Contributor

@Agupane @lukasschor

The checkbox's disabled status should be fixed now.

@ghost
Copy link

ghost commented Nov 25, 2019

Travis automatic deployment:
https://pr296--safereact.review.gnosisdev.com

Storybook book automatic deployment:
https://pr296--safereactstorybook.review.gnosisdev.com

@mmv08
Copy link
Member

mmv08 commented Nov 26, 2019

I prefer to not load those scripts inside the cookie banner to avoid mixing things, I would prefer to do it that separately. And do things like this PR. If you still don't like the idea I can change it

Why not? The entire point of redux is to put state there if we have to share it between few places in our application. In this case, state is consumed only in CookieBanner component.

@Agupane
Copy link
Contributor Author

Agupane commented Nov 26, 2019

It's not, it's consumed also in the google analytics component and will be on the intercom component. Anyways, I'm going to remove that from the store

Agupane and others added 3 commits December 2, 2019 10:44
Replaces localStorage with cookies
Adds js-cookie
… 189-cookie-banner

* '189-cookie-banner' of github.com:gnosis/safe-react:
  Adds cookies utils Replaces localStorage with cookies Adds js-cookie
@ghost
Copy link

ghost commented Dec 2, 2019

Travis automatic deployment:
https://pr296--safereact.review.gnosisdev.com

@ghost
Copy link

ghost commented Dec 2, 2019

Travis automatic deployment:
https://pr296--safereact.review.gnosisdev.com

@Agupane Agupane requested a review from mmv08 December 2, 2019 14:52
Copy link
Member

@mmv08 mmv08 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job 😃

@lukasschor
Copy link
Member

Yes, cookie renewal should be:
365 days for positive
7 days for negative

@ghost
Copy link

ghost commented Dec 3, 2019

Travis automatic deployment:
https://pr296--safereact.review.gnosisdev.com

@mmv08
Copy link
Member

mmv08 commented Dec 3, 2019

Sorry, we forgot about one more requirement. So by law the user should be able to change his consent any time. Because of this, we should have a button inside the interface which would open the cookie banner again. Let's do it in the sidebar at the bottom where we have all the legal links with a label "Cookies"

@ghost
Copy link

ghost commented Dec 3, 2019

Travis automatic deployment:
https://pr296--safereact.review.gnosisdev.com

@Agupane
Copy link
Contributor Author

Agupane commented Dec 3, 2019

Done, please check it now @mikheevm

@ghost
Copy link

ghost commented Dec 3, 2019

Travis automatic deployment:
https://pr296--safereact.review.gnosisdev.com

@lukasschor
Copy link
Member

Works well. However, I think it would be better UX if clicking on "Cookies" would also close the sidebar.

Copy link
Member

@mmv08 mmv08 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think using redux is not really needed because it could be done with react's component state: Sidebar and CookiesBanner components are childs of the same component PageFrame, you could just pass a function from there, but I'm ok with it, not asking for a change.

src/logic/cookies/store/reducer/cookies.js Outdated Show resolved Hide resolved
@ghost
Copy link

ghost commented Dec 4, 2019

Travis automatic deployment:
https://pr296--safereact.review.gnosisdev.com

@Agupane
Copy link
Contributor Author

Agupane commented Dec 4, 2019

It does that now @lukasschor

Works well. However, I think it would be better UX if clicking on "Cookies" would also close the sidebar.

It does that now please check it :)

@ghost
Copy link

ghost commented Dec 4, 2019

Travis automatic deployment:
https://pr296--safereact.review.gnosisdev.com

@Agupane Agupane requested a review from mmv08 December 4, 2019 12:35
@mmv08 mmv08 merged commit 2c42eb5 into development Dec 4, 2019
@mmv08 mmv08 deleted the 189-cookie-banner branch December 4, 2019 12:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants