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.

Sign up for GitHub

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

Minor Demo fixes #5566

Merged
merged 2 commits into from
Dec 6, 2024
Merged

Minor Demo fixes #5566

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
558484c
styling fixes
gilluminate Dec 6, 2024
d19419b
don't watch for modal link on embedded page
gilluminate Dec 6, 2024
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
10 changes: 8 additions & 2 deletions clients/fides-js/src/components/Overlay.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ const Overlay: FunctionComponent<Props> = ({
}, [showBanner, setBannerIsOpen]);

useEffect(() => {
if (!experience || options.modalLinkId === "") {
if (options.fidesEmbed || !experience || options.modalLinkId === "") {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we don't want to be constantly polling for a modal link when the CMP is embedded in the page!

// If empty string is explicitly set, do not attempt to bind the modal link to the click handler.
// developers using `Fides.showModal();` can use this to prevent polling for the modal link.
return () => {};
Expand Down Expand Up @@ -211,7 +211,13 @@ const Overlay: FunctionComponent<Props> = ({
}
window.Fides.showModal = defaultShowModal;
};
}, [options.modalLinkId, options.debug, handleOpenModal, experience]);
}, [
options.fidesEmbed,
options.modalLinkId,
options.debug,
handleOpenModal,
experience,
]);

const handleManagePreferencesClick = (): void => {
handleOpenModal();
Expand Down
6 changes: 3 additions & 3 deletions clients/fides-js/src/components/fides.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -485,7 +485,7 @@ div#fides-consent-content .fides-modal-description {
justify-content: center;
}

.fides-modal-container .fides-button-group-brand {
.fides-modal-footer .fides-button-group-brand {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

.fides-modal-container doesn't exist when things are embedded, so let's target the modal footer class instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this component exists in both the modal and the embed, is it worth trying to rename it to something more generic or would that break too much stuff?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

which component are you referring to, the footer or the brand?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, the modal footer, that is. Or anything else that's getting used here that has modal in the name, I suppose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think eventually we'll want to rename everything as "layer1" and "layer2" instead for that reason. I don't think this now is the time to do that. 👍

min-height: var(--fides-overlay-modal-secondary-button-group-height);
}

Expand Down Expand Up @@ -1023,13 +1023,13 @@ div#fides-overlay-wrapper .fides-toggle .fides-toggle-display {
position: relative;
}

.fides-modal-container .fides-i18n-menu {
.fides-modal-footer .fides-i18n-menu {
Copy link
Contributor

Choose a reason for hiding this comment

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

Screenshot 2024-12-06 at 13 52 34

These styles still don't seem to be applying for me and the i18n menu is still centered, is that intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

weird, that was working for me. Try rebuilding/clearing your cache?

position: absolute;
left: var(--fides-overlay-padding);
bottom: var(--fides-overlay-padding);
}

.fides-modal-container .fides-button-group-i18n {
.fides-modal-footer .fides-button-group-i18n {
min-height: var(--fides-overlay-modal-secondary-button-group-height);
}

Expand Down
12 changes: 11 additions & 1 deletion clients/sample-app/src/pages/embedded-consent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,17 @@ const IndexPage = ({ gtmContainerId, privacyCenterUrl }: Props) => {
{/* Require FidesJS to "embed" it's UI onto the page, instead of as an overlay over the <body> itself. (see https://ethyca.com/docs/dev-docs/js/reference/interfaces/FidesOptions#fides_embed) */}
<script>{`window.fides_overrides = { fides_embed: true, fides_disable_banner: true }`}</script>
{/* Allow the embedded consent modal to fill the screen */}
<style>{`#fides-embed-container { --fides-overlay-width: 'auto' }`}</style>
<style>{`
#fides-embed-container {
--fides-overlay-width: 'auto'
}

body {
font-family: "Inter", sans-serif;
color: #171923;
font-size: 14px;
}
Comment on lines +68 to +72
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fonts were all wonky on this page otherwise. This mimics the font choices of the Cookie House homepage.

`}</style>
</Head>
{/**
Insert the fides.js script and run the GTM integration once ready
Expand Down
Loading