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

[fides.js] Fix button arrangement #4407

Merged
merged 13 commits into from
Nov 10, 2023
Merged

Conversation

allisonking
Copy link
Contributor

@allisonking allisonking commented Nov 8, 2023

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

Description Of Changes

Privacy policy is now always in the middle, per this design: https://www.figma.com/file/dmEwdK3xZwjKfGVQ9t66xe/Fides-v.2-Master-Working-Files?type=design&node-id=11310-269887&mode=design&t=OnhFjNC97D3WXHL9-0

This simplifies the code considerably! Also fixes some alignment issues.

https://www.loom.com/share/d27a84f9a8a241c4bbe43e22a13c5500?sid=bee861e1-df2c-4747-9427-ce5da6022d2f

Code Changes

  • Have ConsentBanner.tsx be responsible for rendering the privacy policy instead of parent components passing it in as a middle button or as a child. Since the policy is always in the middle now, this is feasible and cleans the code up a bit!

Steps to Confirm

  • See loom—test out having a privacy policy and not having a privacy policy, at different viewport sizes, and for both TCF and notices

Pre-Merge Checklist

  • All CI Pipelines Succeeded
  • Issue Requirements are Met
  • Update CHANGELOG.md

Copy link

vercel bot commented Nov 8, 2023

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

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
fides-plus-nightly ⬜️ Ignored (Inspect) Visit Preview Nov 10, 2023 9:27pm

Copy link

cypress bot commented Nov 8, 2023

Passing run #5211 ↗︎

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 14176fd into 24e9142...
Project: fides Commit: 382d8d9150 ℹ️
Status: Passed Duration: 01:21 💡
Started: Nov 10, 2023 9:46 PM Ended: Nov 10, 2023 9:47 PM

Review all test suite changes for PR #4407 ↗︎

@allisonking allisonking marked this pull request as ready for review November 8, 2023 22:16
@galvana
Copy link
Contributor

galvana commented Nov 9, 2023

Code-wise this looks good, but I noticed some spacing issues

The buttons have some left padding that causes the Privacy Policy to be a little off center
image

The TCF modal seems to have more padding on the right side than the left
image

Comment on lines -73 to -74
children?: ComponentChildren;
middleButton?: VNode;
Copy link
Contributor

Choose a reason for hiding this comment

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

🙌 I'm glad these are gone!

@allisonking
Copy link
Contributor Author

Thanks @galvana , that's a good catch! I'll fix the opt in/out button padding 👍

I'm unable to reproduce the modal padding being different though... I wonder if it has to do with that scrollbar thing?
image

@allisonking
Copy link
Contributor Author

Ah yeah I just turned my "Show scroll bars" in osx settings to "Always" and now I see it...
image

@allisonking
Copy link
Contributor Author

@galvana it appears that removing scrollbar-gutter: stable from .fides-modal-body fixes it for me, but I know you added that for a reason... but I forget what it was!

@allisonking
Copy link
Contributor Author

Fixed the button padding issue! c0b6e0b
image

@galvana
Copy link
Contributor

galvana commented Nov 10, 2023

@galvana it appears that removing scrollbar-gutter: stable from .fides-modal-body fixes it for me, but I know you added that for a reason... but I forget what it was!

@allisonking, it was so that the modal description didn't flicker when going from a tab with overflow content to one without. I think we can just move scrollbar-gutter: stable from fides-modal-body to the new outermost container, fides-consent-content. It worked for me locally

scroll

Copy link
Contributor

@galvana galvana left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes, this looks ready to me!

@allisonking allisonking merged commit f71b866 into main Nov 10, 2023
13 checks passed
@allisonking allisonking deleted the aking/prod-1339/btn-arrangement branch November 10, 2023 21:48
eastandwestwind pushed a commit that referenced this pull request Nov 12, 2023
Co-authored-by: Adrian Galvan <adrian@ethyca.com>
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