feat(CookieConsent): add CookieConsent component #123

Lakate · 2023-11-24T13:19:10Z

No description provided.

gravity-ui-bot · 2023-11-24T13:20:37Z

Lakate · 2023-11-24T13:22:59Z

https://preview.gravity-ui.com/components/123/?path=/story/components-cookieconsent--simple-consent

ValeraS · 2023-11-28T00:18:08Z

src/components/CookieConsent/ConsentManager.ts

+    sameSite: true,
+};
+
+export enum ConsentType {


Let's not use enums, or at least let's not force users to use them.

Could you explain please
I see that we already have enums in components and even export them

ValeraS · 2023-11-28T00:20:03Z

src/components/CookieConsent/ConsentManager.ts

@@ -0,0 +1,164 @@
+import _ from 'lodash';


Suggested change

import _ from 'lodash';

import pick from 'lodash/pick';

ValeraS · 2023-11-28T00:27:06Z

src/components/CookieConsent/CookieConsent.tsx

@@ -0,0 +1,100 @@
+import React, {useEffect} from 'react';


We enforce this React import syntax in uikit and plan to do so here.

Suggested change

import React, {useEffect} from 'react';

import React from 'react';

ValeraS · 2023-11-28T00:28:41Z

src/components/CookieConsent/CookieConsent.tsx

+        }, 1000);
+
+        return () => clearTimeout(timeoutId);
+    }, [disableInitialOpen]);


Not all dependencies are listed

ValeraS · 2023-11-28T00:38:51Z

src/components/CookieConsent/components/Collapse/Collapse.tsx

+    );
+}
+
+export function Collapse({


Can we use Disclosure component form uikit?

ValeraS · 2023-11-28T00:45:15Z

src/components/CookieConsent/components/ConsentPopup/ConsentPopup.tsx

+                                <span
+                                    className={b('link')}
+                                    onClick={onChangeStep(ConsentPopupStep.Manage)}
+                                    onKeyDown={onChangeStep(ConsentPopupStep.Manage)}


When you press any key, for example tab, you go to the next step.

ValeraS · 2023-11-28T00:47:59Z

src/components/CookieConsent/components/FoldableList/FoldableList.tsx

+                                            event.stopPropagation();
+                                        }}
+                                    >
+                                        <Checkbox


You should not place interactive content inside a button; it is not accessible.

ValeraS · 2023-11-28T08:10:00Z

src/components/CookieConsent/components/SimpleConsent/SimpleConsent.tsx

+        onClose,
+    } = props;
+
+    const onClick = useCallback(


Most of the use of useCallback is unnecessarily

ValeraS · 2023-11-28T08:10:52Z

src/components/CookieConsent/CookieConsent.tsx

+
+    useEffect(() => {
+        if (!isNotificationMode) {
+            consentManager.subscribe(() => {


Where does unsubscription take place?

ValeraS · 2023-11-28T08:17:39Z

src/components/CookieConsent/ConsentManager.ts

+        return this.consentEdition;
+    }
+
+    get projectCEdition() {


maybe better projectEdition?

ValeraS · 2023-12-01T12:50:46Z

src/components/CookieConsent/README.md

+    // do something: e.g. sent events to analytics
+  }, []);
+
+  React.useMount(() => {


What is useMount? I don't know such a hook in React.

seems like forgot to fix, thank you
fixed

ValeraS · 2023-12-01T13:06:42Z

src/components/CookieConsent/components/ConsentPopup/ConsentPopup.tsx

+    );
+
+    React.useEffect(() => {
+        setCurrentConsents(getCurrentConsents(consentManager));


Do we need also subscribe to changes of consents?

yes, we need for managing visibility

ValeraS · 2023-12-01T13:07:21Z

src/components/CookieConsent/components/ConsentPopup/ConsentPopup.tsx

+
+const b = block('consent-popup');
+
+const getCurrentConsents = (consentManager: ConsentManager) => {


Why this is not a logic of consentManager itself?

In consentManager we have getConsents that returns actual consents. If we haven't defined consetns yet, we need to show consent component

But in ConsentPopup we need to define all consent types because we need to show them in FoldableList

ValeraS · 2023-12-01T13:12:22Z

src/components/CookieConsent/components/ConsentPopup/ConsentPopup.tsx

+        return getCurrentConsents(consentManager);
+    });
+    const [currentStep, setCurrentStep] = React.useState<`${ConsentPopupStep}`>(step);
+    const onChangeStep = React.useCallback(


Why do we need useCallback here?

ValeraS · 2023-12-01T13:12:30Z

src/components/CookieConsent/components/ConsentPopup/ConsentPopup.tsx

+            };
+        });
+    }, [cookieList, currentConsents]);
+    const onChoose = React.useCallback(


Why do we need useCallback here?

ValeraS · 2023-12-01T13:44:23Z

src/components/CookieConsent/helpers.ts

+    value: boolean,
+    cookiesTypes: `${ConsentType}`[],
+    onlyNecessary?: boolean,


Strange interface, if onlyNecessary=true, then value is not used, it's not obvious. Seems better:

function prepareConsent(consent: 'All' | 'None' | 'onlyNecessary', cookiesTypes)

and why is this not part of the ConsentManager

move to ConsentManager and change interface

ValeraS · 2023-12-01T14:00:47Z

src/components/CookieConsent/CookieConsent.tsx

+            const isConsentsDefined = consentManager.isAllConsentsDefined();
+            const shouldOpen = isNotificationMode
+                ? !consentManager.isClosed() || !isConsentsDefined
+                : !isConsentsDefined;
+            const differentEdition = consentManager.projectEdition !== consentManager.edition;


Seems, this logic should be inside consentManager

Disagree, consentManager works only with cookies and managing them
Logic about component visibility should be inside the component

ValeraS · 2023-12-01T14:03:57Z

src/components/CookieConsent/CookieConsent.tsx

+        return consentManager.subscribe(() => {
+            setIsOpened(!consentManager.isAllConsentsDefined());
+        });
+    }, [consentManager, isNotificationMode]);


How does it depend on isNotificationMode?

ValeraS · 2023-12-01T14:07:32Z

src/components/CookieConsent/CookieConsent.tsx

+    consentManager,
+    onConsentPopupClose,
+    disableInitialOpen,
+    onAction,


What is the use case for this props?

For now - we use it in storybook
But anyone can use it if wants to have own callback

ValeraS · 2023-12-01T14:25:19Z

src/components/CookieConsent/README.md

+    step?: `${ConsentPopupStep}`;
+    cookieList?: ConsentPopupCookieListItem[];
+    /* To open by the link or button, so the popup should not be opened at this moment */
+    forceOpenManageStep?: boolean;


I can't find where this is used?

ValeraS · 2023-12-05T09:51:27Z

src/components/CookieConsent/ConsentManager.ts

+    private readonly subscribers: Subscriber[] = [];
+    private readonly cookieSettings: CookieSettings = CONSENT_COOKIE_SETTINGS;
+
+    constructor(mode: `${ConsentMode}`, edition?: number) {


Suggested change

constructor(mode: `${ConsentMode}`, edition?: number) {

constructor({

mode,

edition,

cookieSettings = CONSENT_COOKIE_SETTINGS

}: {mode: `${ConsentMode}`, edition?: number, cookieSettings?: CookieSettings}) {

ValeraS · 2023-12-05T09:53:56Z

src/components/CookieConsent/ConsentManager.ts

+    setCookieSettings(settings: Partial<CookieSettings>) {
+        Object.assign(this.cookieSettings, settings);
+    }


Suggested change

setCookieSettings(settings: Partial<CookieSettings>) {

Object.assign(this.cookieSettings, settings);

}

I suggest setting it via constructor.

ValeraS · 2023-12-05T09:59:27Z

src/components/CookieConsent/ConsentManager.ts

+    set mode(newMode: `${ConsentMode}`) {
+        this.consentMode = newMode;
+    }


Suggested change

set mode(newMode: `${ConsentMode}`) {

this.consentMode = newMode;

}

With current use of ConsentManager it should be done via creating a new ConsentManager.

consentManager should be only one in service

This method is necessary for this scenario: in Notification mode we still want to provide the ability to manage settings. Therefore, when we want to open ConsentPopup on manage step we switch the mode and open it

ValeraS · 2023-12-05T10:08:21Z

src/components/CookieConsent/ConsentManager.ts

+        return this.cookieSettings;
+    }
+
+    getConsents() {


I still think that this should return the same as getCurrentConsents.

ValeraS · 2023-12-05T10:11:21Z

src/components/CookieConsent/ConsentManager.ts

+        };
+    }
+
+    setConsents(consents: Consents) {


How about this:

Suggested change

setConsents(consents: Consents) {

setConsents(consents: Consents | 'All' | 'OnlyNecessary') {

and make prepareConsent private?

ValeraS · 2023-12-05T10:21:36Z

src/components/CookieConsent/CookieConsent.tsx

+
+    const onConsentPopupAction = (consents: Consents) => {
+        consentManager.setConsents(consents);
+        setIsOpened(false);


onAction is always called with onClose

add onConsentPopupClose in onConsentPopupAction

ValeraS · 2023-12-05T10:24:16Z

src/components/CookieConsent/components/ConsentPopup/ConsentPopup.tsx

+    const onButtonClick = (onlyNecessary?: boolean) => {
+        return () => {
+            onAction(consentManager.prepareConsent(onlyNecessary ? 'OnlyNecessary' : 'All'));
+            onClose?.();


onClose is not optional.

ValeraS · 2023-12-05T10:24:21Z

src/components/CookieConsent/components/ConsentPopup/ConsentPopup.tsx

+    };
+    const confirmSelectedConsent = () => {
+        onAction(currentConsents);
+        onClose?.();


onClose is not optional.

ValeraS · 2023-12-05T10:24:42Z

src/components/CookieConsent/components/SimpleConsent/SimpleConsent.tsx

+    const onClick = (isAll: boolean) => {
+        return () => {
+            onAction(consentManager.prepareConsent(isAll ? 'All' : 'OnlyNecessary'));
+            onClose?.();


onClose is not optional.

deleted from SimpleConsent

ValeraS · 2023-12-05T10:48:57Z

src/components/CookieConsent/ConsentManager.ts

+            newValue = {
+                ...newValue,
+                [AdditionalConsentParams.Closed]: true,
+            };


Suggested change

newValue = {

...newValue,

[AdditionalConsentParams.Closed]: true,

};

newValue[AdditionalConsentParams.Closed] = true;

Lakate requested review from amje, ValeraS and korvin89 as code owners November 24, 2023 13:19

Lakate force-pushed the feat/cookie-consent branch from 5f6202f to 59df049 Compare November 24, 2023 13:22

Lakate force-pushed the feat/cookie-consent branch from 59df049 to 55bd842 Compare November 27, 2023 14:24

ValeraS requested changes Nov 28, 2023

View reviewed changes

Lakate force-pushed the feat/cookie-consent branch 5 times, most recently from 27b7e6a to 5dfa8df Compare December 1, 2023 10:04

Lakate requested review from DarkGenius, NikitaCG, niktverd, Marginy605, ogonkov and KirillDyachkovskiy as code owners December 1, 2023 10:04

Lakate force-pushed the feat/cookie-consent branch 2 times, most recently from 951f5fe to db5ebb5 Compare December 1, 2023 12:12

korvin89 approved these changes Dec 1, 2023

View reviewed changes

Lakate force-pushed the feat/cookie-consent branch 2 times, most recently from bc4581b to 761b66f Compare December 1, 2023 12:38

ValeraS requested changes Dec 1, 2023

View reviewed changes

Lakate force-pushed the feat/cookie-consent branch 3 times, most recently from b525933 to 98073fc Compare December 4, 2023 13:01

ValeraS requested changes Dec 5, 2023

View reviewed changes

Lakate force-pushed the feat/cookie-consent branch from 98073fc to fc98d41 Compare December 5, 2023 12:27

Lakate force-pushed the feat/cookie-consent branch 7 times, most recently from b60ebbc to 56bebb1 Compare December 6, 2023 13:03

feat(CookieConsent): add CookieConsent component

108205e

Lakate force-pushed the feat/cookie-consent branch from 56bebb1 to 108205e Compare December 6, 2023 13:10

ValeraS approved these changes Dec 6, 2023

View reviewed changes

Lakate merged commit 1572965 into main Dec 6, 2023
3 checks passed

Lakate deleted the feat/cookie-consent branch December 6, 2023 13:16

gravity-ui-bot mentioned this pull request Dec 6, 2023

chore(main): release 2.9.0 #126

Merged

	import React, {useEffect} from 'react';
	import React from 'react';


		const b = block('consent-popup');

		const getCurrentConsents = (consentManager: ConsentManager) => {

-    constructor(mode: `${ConsentMode}`, edition?: number) {
+    constructor({
+        mode,
+        edition,
+        cookieSettings = CONSENT_COOKIE_SETTINGS
+    }: {mode: `${ConsentMode}`, edition?: number, cookieSettings?: CookieSettings}) {

	setCookieSettings(settings: Partial<CookieSettings>) {
	Object.assign(this.cookieSettings, settings);
	}

	set mode(newMode: `${ConsentMode}`) {
	this.consentMode = newMode;
	}

	setConsents(consents: Consents) {
	setConsents(consents: Consents \| 'All' \| 'OnlyNecessary') {

feat(CookieConsent): add CookieConsent component #123

feat(CookieConsent): add CookieConsent component #123

Conversation

Lakate commented Nov 24, 2023

gravity-ui-bot commented Nov 24, 2023

Lakate commented Nov 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lakate Dec 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lakate Dec 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lakate Dec 5, 2023 •

edited

Loading

Lakate Dec 5, 2023 •

edited

Loading