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

TCF experience design improvements #4222

Merged
merged 18 commits into from
Oct 10, 2023
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion clients/fides-js/rollup.config.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import postcss from "rollup-plugin-postcss";

const NAME = "fides";
const IS_DEV = process.env.NODE_ENV === "development";
const GZIP_SIZE_ERROR_KB = 20; // fail build if bundle size exceeds this
const GZIP_SIZE_ERROR_KB = 22; // fail build if bundle size exceeds this
galvana marked this conversation as resolved.
Show resolved Hide resolved
const GZIP_SIZE_WARN_KB = 15; // log a warning if bundle size exceeds this

// TCF
Expand Down
54 changes: 34 additions & 20 deletions clients/fides-js/src/components/ConsentBanner.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { h, FunctionComponent, ComponentChildren } from "preact";
import { h, FunctionComponent, ComponentChildren, VNode } from "preact";
import { getConsentContext } from "../lib/consent-context";
import { ExperienceConfig } from "../lib/consent-types";
import CloseButton from "./CloseButton";
Expand All @@ -9,8 +9,10 @@ interface BannerProps {
experience: ExperienceConfig;
onClose: () => void;
bannerIsOpen: boolean;
children: ComponentChildren;
children?: ComponentChildren;
onVendorPageClick?: () => void;
allisonking marked this conversation as resolved.
Show resolved Hide resolved
buttonGroup: VNode;
bannerType?: String;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't even know there was a String! looks like we should prefer string tho: https://stackoverflow.com/questions/14727044/what-is-the-difference-between-types-string-and-string

Suggested change
bannerType?: String;
bannerType?: string;

Copy link
Contributor

Choose a reason for hiding this comment

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

or even better, would be to restrict this to the strings it can be, i.e. "TCF"|"notices"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This ended up going away after refactoring the GPC check


const ConsentBanner: FunctionComponent<BannerProps> = ({
Expand All @@ -19,6 +21,8 @@ const ConsentBanner: FunctionComponent<BannerProps> = ({
bannerIsOpen,
children,
onVendorPageClick,
buttonGroup,
bannerType,
}) => {
const showGpcBadge = getConsentContext().globalPrivacyControl;
return (
Expand All @@ -31,27 +35,37 @@ const ConsentBanner: FunctionComponent<BannerProps> = ({
<div id="fides-banner">
<div id="fides-banner-inner">
<CloseButton ariaLabel="Close banner" onClick={onClose} />
<div id="fides-banner-heading">
<div id="fides-banner-title" className="fides-banner-title">
{experience.title}
</div>
{showGpcBadge ? (
<GpcBadge
label="Global Privacy Control Signal"
status="detected"
/>
) : null}
</div>
<div
id="fides-banner-description"
className="fides-banner-description"
id="fides-banner-inner-container"
style={{
gridTemplateColumns: children ? "1fr 1fr" : "1fr",
}}
galvana marked this conversation as resolved.
Show resolved Hide resolved
>
<ExperienceDescription
description={experience.description}
onVendorPageClick={onVendorPageClick}
/>
<div id="fides-banner-inner-description">
<div id="fides-banner-heading">
<div id="fides-banner-title" className="fides-banner-title">
{experience.title}
</div>
{showGpcBadge && bannerType !== "TCF" ? (
<GpcBadge
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Passing in bannerType to hide the GpcBadge for the TCF banner/modal

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm if its only purpose is whether or not to show the gpc badge, maybe instead of bannerType we make the prop hideGpc, then change above. I think that makes it a bit clearer for someone using this component exactly what that prop is going to do

const showGpcBadge = getConsentContext().globalPrivacyControl && !hideGpc;

Copy link
Contributor

Choose a reason for hiding this comment

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

actually maybe a better idea is to modify the function getGlobalPrivacyControl to query window.Fides.options.tcfEnabled. if it's enabled, return false. then I don't think gpc would ever render for TCF, and it'd be tied to gpc logic, as opposed to component logic, which is nice!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like that, much cleaner!

label="Global Privacy Control Signal"
status="detected"
/>
) : null}
</div>
<div
id="fides-banner-description"
className="fides-banner-description"
>
<ExperienceDescription
description={experience.description}
onVendorPageClick={onVendorPageClick}
/>
</div>
</div>
{children}
{buttonGroup}
</div>
{children}
</div>
</div>
</div>
Expand Down
25 changes: 21 additions & 4 deletions clients/fides-js/src/components/ConsentButtons.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { h, VNode } from "preact";
import { ComponentChildren, h, VNode } from "preact";
import Button from "./Button";
import {
ButtonType,
Expand All @@ -12,26 +12,35 @@ export const ConsentButtons = ({
experienceConfig,
onManagePreferencesClick,
firstButton,
middleButton,
onAcceptAll,
onRejectAll,
children,
}: {
experienceConfig: ExperienceConfig;
onManagePreferencesClick?: () => void;
firstButton?: VNode;
middleButton?: VNode;
onAcceptAll: () => void;
onRejectAll: () => void;
children: ComponentChildren;
}) => (
<div id="fides-button-group">
{onManagePreferencesClick ? (
<div>
<div style={{ display: "flex" }}>
<Button
buttonType={ButtonType.TERTIARY}
label={experienceConfig.privacy_preferences_link_label}
onClick={onManagePreferencesClick}
/>
</div>
) : null}
<div className={firstButton ? "fides-modal-button-group" : undefined}>
{middleButton || null}
<div
className={
firstButton ? "fides-modal-button-group" : "fides-banner-button-group"
}
>
{firstButton || null}
<Button
buttonType={ButtonType.PRIMARY}
Expand All @@ -44,6 +53,7 @@ export const ConsentButtons = ({
onClick={onAcceptAll}
/>
</div>
{children}
</div>
);

Expand All @@ -56,6 +66,8 @@ interface NoticeConsentButtonProps {
enabledKeys: NoticeKeys;
isAcknowledge: boolean;
isInModal?: boolean;
children?: ComponentChildren;
middleButton?: VNode;
allisonking marked this conversation as resolved.
Show resolved Hide resolved
}

export const NoticeConsentButtons = ({
Expand All @@ -65,6 +77,8 @@ export const NoticeConsentButtons = ({
enabledKeys,
isInModal,
isAcknowledge,
children,
middleButton,
}: NoticeConsentButtonProps) => {
if (!experience.experience_config || !experience.privacy_notices) {
return null;
Expand Down Expand Up @@ -118,6 +132,9 @@ export const NoticeConsentButtons = ({
/>
) : undefined
}
/>
middleButton={middleButton}
>
{children}
</ConsentButtons>
);
};
15 changes: 3 additions & 12 deletions clients/fides-js/src/components/ConsentModal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,13 @@ const ConsentModal = ({
experience,
children,
onVendorPageClick,
modalType,
}: {
attributes: Attributes;
experience: ExperienceConfig;
children: ComponentChildren;
onVendorPageClick?: () => void;
modalType?: String;
Copy link
Contributor

Choose a reason for hiding this comment

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

same thing here—see my meandering comment thoughts about on bannerType

}) => {
const { container, overlay, dialog, title, closeButton } = attributes;

Expand Down Expand Up @@ -49,19 +51,8 @@ const ConsentModal = ({
description={experience.description}
/>
</p>
<GpcInfo />
{modalType !== "TCF" && <GpcInfo />}
{children}
{experience.privacy_policy_link_label &&
allisonking marked this conversation as resolved.
Show resolved Hide resolved
experience.privacy_policy_url ? (
<a
href={experience.privacy_policy_url}
rel="noopener noreferrer"
target="_blank"
className="fides-modal-privacy-policy"
>
{experience.privacy_policy_link_label}
</a>
) : null}
</div>
</div>
);
Expand Down
5 changes: 4 additions & 1 deletion clients/fides-js/src/components/Overlay.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ interface Props {
renderBanner: (props: RenderBannerProps) => VNode | null;
renderModalContent: (props: RenderModalContent) => VNode;
onVendorPageClick?: () => void;
overlayType?: String;
}

const Overlay: FunctionComponent<Props> = ({
Expand All @@ -42,6 +43,7 @@ const Overlay: FunctionComponent<Props> = ({
renderBanner,
renderModalContent,
onVendorPageClick,
overlayType,
}) => {
const delayBannerMilliseconds = 100;
const delayModalLinkMilliseconds = 200;
Expand All @@ -54,7 +56,7 @@ const Overlay: FunctionComponent<Props> = ({

const { instance, attributes } = useA11yDialog({
id: "fides-modal",
role: "dialog",
role: "alertdialog",
galvana marked this conversation as resolved.
Show resolved Hide resolved
title: experience?.experience_config?.title || "",
onClose: dispatchCloseEvent,
});
Expand Down Expand Up @@ -153,6 +155,7 @@ const Overlay: FunctionComponent<Props> = ({
attributes={attributes}
experience={experience.experience_config}
onVendorPageClick={onVendorPageClick}
modalType={overlayType}
>
{renderModalContent({ onClose: handleCloseModal })}
</ConsentModal>
Expand Down
36 changes: 36 additions & 0 deletions clients/fides-js/src/components/PrivacyPolicyLink.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import { h } from "preact";
import { ExperienceConfig } from "../lib/consent-types";

const PrivacyPolicyLink = ({
experience,
}: {
experience?: ExperienceConfig;
}) => {
if (
!experience?.privacy_policy_link_label ||
!experience?.privacy_policy_url
) {
return null;
}

return (
<div
style={{
display: "flex",
alignItems: "center",
justifyContent: "center",
}}
>
<a
href={experience.privacy_policy_url}
rel="noopener noreferrer"
target="_blank"
className="fides-privacy-policy"
>
{experience.privacy_policy_link_label}
</a>
</div>
);
};

export default PrivacyPolicyLink;
4 changes: 3 additions & 1 deletion clients/fides-js/src/components/Toggle.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,9 @@ const Toggle = ({
disabled={disabled}
/>
{/* Mark as `hidden` so it will fall back to a regular checkbox if CSS is not available */}
<span className="fides-toggle-display" hidden />
<span className="fides-toggle-display" hidden>
{checked ? "Opt-in" : "Opt-out"}
</span>
</label>
);
};
Expand Down
Loading
Loading