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

Jump to bottom

PROD-1277 Refactor embedded mode to not use A11y dialog #4355

Merged
merged 6 commits into from
Oct 31, 2023
Merged

PROD-1277 Refactor embedded mode to not use A11y dialog #4355

merged 6 commits into from
Oct 31, 2023

Conversation

eastandwestwind
Copy link
Contributor

@eastandwestwind eastandwestwind commented Oct 30, 2023 •
edited by allisonking
Loading

Closes https://ethyca.atlassian.net/browse/PROD-1277

Description Of Changes

Refactor embedded mode to not use A11y dialog.

Code Changes

  • Adds new shared component ConsentContent.tsx that can be called without needing the A11y dialog
  • Updates CSS to support the refactor

Steps to Confirm

  • Regression test the consent modal for styling issues. For each of the TCF and non-TCF consent modals, test with and without ?fides_embed=true. Test in desktop and responsive mode too. See Loom for reference:

https://www.loom.com/share/b68e6001f2bb49ffa3904d4324446643

Pre-Merge Checklist

@eastandwestwind eastandwestwind force-pushed the PROD-1277-embedded-modal-a11y branch from 1d33f8c to adb5efb Compare October 30, 2023 15:16
@cypress
Copy link

cypress bot commented Oct 30, 2023 •
edited
Loading

Passing run #4903 ↗︎

0 4 0 0 Flakiness 0
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Merge 20057de into 21d10c9...
Project: fides Commit: 60c766955a ℹ️
Status: Passed Duration: 01:00 💡
Started: Oct 30, 2023 5:33 PM Ended: Oct 30, 2023 5:34 PM

Review all test suite changes for PR #4355 ↗︎

allisonking
allisonking approved these changes Oct 30, 2023
View reviewed changes
Copy link
Contributor

@allisonking allisonking left a comment

Choose a reason for hiding this comment

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

tested locally and this works great! the refactor turned out to be clean too, nice job 😎 once the cypress test is fixed, I think this should be good to merge 👍

allisonking
allisonking approved these changes Oct 30, 2023
View reviewed changes
Copy link
Contributor

@allisonking allisonking left a comment

Choose a reason for hiding this comment

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

💥

@eastandwestwind eastandwestwind merged commit a0e6f6d into main Oct 31, 2023
@eastandwestwind eastandwestwind deleted the PROD-1277-embedded-modal-a11y branch October 31, 2023 13:25
Kelsey-Ethyca pushed a commit that referenced this pull request Nov 1, 2023
@eastandwestwind @Kelsey-Ethyca
PROD-1277 Refactor embedded mode to not use A11y dialog (#4355)
1be266e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@allisonking allisonking allisonking approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@eastandwestwind @allisonking