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

Privacy experience form #3219

Merged
merged 11 commits into from
May 8, 2023
Merged

Conversation

allisonking
Copy link
Contributor

@allisonking allisonking commented May 4, 2023

Closes #3126

Code Changes

  • Add new PrivacyExperienceForm.tsx
    • This one has a few subcomponents which control the various form rendering logic from privacy experiences
    • Other than that, it's pretty similar to PrivacyNoticeForm.tsx
  • Cypress tests, including a handy function to easily test different experiences backed by varying notices

Steps to Confirm

Pre-Merge Checklist

Description Of Changes

https://www.loom.com/share/556bc39d3f6b4f7ca50a4c2cef14929a

Screen.Recording.2023-05-04.at.11.31.28.AM.mov

@allisonking allisonking force-pushed the aking/3126/privacy-experience-form branch from b27ad02 to 412739c Compare May 4, 2023 13:54
@cypress
Copy link

cypress bot commented May 4, 2023

Passing run #1786 ↗︎

0 4 0 0 Flakiness 0

Details:

Merge af2e44a into 2afdfea...
Project: fides Commit: b3518609d2 ℹ️
Status: Passed Duration: 00:48 💡
Started: May 5, 2023 7:49 PM Ended: May 5, 2023 7:50 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@allisonking allisonking marked this pull request as ready for review May 4, 2023 17:06
Copy link
Contributor

@TheAndrewJackson TheAndrewJackson left a comment

Choose a reason for hiding this comment

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

I played around with all the example experiences the original PR had and was able to edit everything as expected. I like the implementation for dynamically showing different parts of the form.

I think it was a good choice to not attempt a dynamic validation schema like I did with the custom fields 😅 At least not for the first pass. I wish that formik supported that pattern better.

:shipit:

@allisonking
Copy link
Contributor Author

I played around with all the example experiences the original PR had and was able to edit everything as expected. I like the implementation for dynamically showing different parts of the form.

I think it was a good choice to not attempt a dynamic validation schema like I did with the custom fields 😅 At least not for the first pass. I wish that formik supported that pattern better.

:shipit:

haha yeah, I thought I could get away with no ValidationSchema, but it turns out we probably do want some validation #3226 so I might have to restructure how this works

@allisonking allisonking force-pushed the aking/3122/privacy-experiences-initial-page branch from 878b7e8 to f90a54a Compare May 5, 2023 15:49
Base automatically changed from aking/3122/privacy-experiences-initial-page to main May 5, 2023 17:15
@allisonking allisonking force-pushed the aking/3126/privacy-experience-form branch from 2321456 to c617ed7 Compare May 5, 2023 17:43
@allisonking
Copy link
Contributor Author

I refactored the form logic a bit in order to accommodate required fields (somewhat dynamic validation schemas—not to the same extent you had to do @TheAndrewJackson in custom fields, since they don't change on user input, just depending on what data is received). I added a hook useExperienceForm to encapsulate the various rule logic, so that all rules can hang out in one place, which hopefully makes adding/editing/removing rules easier

Copy link
Contributor

@TheAndrewJackson TheAndrewJackson left a comment

Choose a reason for hiding this comment

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

Solid refactor. The new hook cleans the logic up a lot. It's helpful that the validation logic doesn't need to change based on a drop down option! It's still dynamic but not as dynamic as the custom fields form which is nice.

@allisonking
Copy link
Contributor Author

I had a suspicion my hook was being called more than I wanted it to be and it turned out to be true! so I refactored one more time 😅 this time rules are passed around as props, but I think it works pretty well since the same set of rules get passed everywhere

@allisonking allisonking merged commit 58338f1 into main May 8, 2023
@allisonking allisonking deleted the aking/3126/privacy-experience-form branch May 8, 2023 13:16
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.

Individual experience form page
2 participants