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

Minor Demo fixes #5566

Merged
merged 2 commits into from
Dec 6, 2024
Merged

Minor Demo fixes #5566

merged 2 commits into from
Dec 6, 2024

Conversation

gilluminate
Copy link
Contributor

Some minor fixes for the Embedded Demo page.

Copy link

vercel bot commented Dec 6, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
fides-plus-nightly ⬜️ Ignored (Inspect) Dec 6, 2024 0:29am

@gilluminate gilluminate requested a review from jpople December 6, 2024 00:29
@@ -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!

@@ -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. 👍

Comment on lines +68 to +72
body {
font-family: "Inter", sans-serif;
color: #171923;
font-size: 14px;
}
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.

Copy link

cypress bot commented Dec 6, 2024

fides    Run #11312

Run Properties:  status check passed Passed #11312  •  git commit b9d49376eb ℹ️: Merge d19419b171c0f69125f18da2c906d9a6c377dc24 into ca8e8470f38218fd95d448e1cf64...
Project fides
Branch Review refs/pull/5566/merge
Run status status check passed Passed #11312
Run duration 00m 54s
Commit git commit b9d49376eb ℹ️: Merge d19419b171c0f69125f18da2c906d9a6c377dc24 into ca8e8470f38218fd95d448e1cf64...
Committer Jason Gill
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 4
View all changes introduced in this branch ↗︎

@@ -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?

@gilluminate gilluminate merged commit c08f5ee into main Dec 6, 2024
13 checks passed
@gilluminate gilluminate deleted the gill/minor-css-fixes-to-embeded-demo branch December 6, 2024 22:58
Copy link

cypress bot commented Dec 6, 2024

fides    Run #11342

Run Properties:  status check passed Passed #11342  •  git commit c08f5ee834: Minor Demo fixes (#5566)
Project fides
Branch Review main
Run status status check passed Passed #11342
Run duration 00m 48s
Commit git commit c08f5ee834: Minor Demo fixes (#5566)
Committer Jason Gill
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 4
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.
View all changes introduced in this branch ↗︎

andres-torres-marroquin pushed a commit that referenced this pull request Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants