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

Modal css fixes #3429

Merged
merged 3 commits into from
Jun 2, 2023
Merged

Conversation

allisonking
Copy link
Contributor

@allisonking allisonking commented Jun 1, 2023

Closes #3423

Code Changes

  • Fix padding issues. This was sneaky because browsers include their own margins on certain elements by default. This didn't become apparent until using it in something like a real next app which probably has a css reset to undo all browser defaults.
  • Unnest CSS selectors. It turns out this isn't widely supported, resulting in much of the modal CSS not rendering at all in firefox and safari!

Steps to Confirm

Pre-Merge Checklist

Description Of Changes

Chrome:

image

Firefox:

image

Safari:

image

Note that the borders are straight because we only have the full width version of the banner right now. We haven't implemented break points for the banner yet, but when it gets smaller, the border should start to curve (see this slack convo)

A note on CSS resets:

Apps can normally do something like

* {
  margin: 0
}

We can't do that, or else we might mess up the host app's CSS! we could do

#fides-overlay {
  * {
    margin: 0
  }
}

but that's nested CSS so isn't widely supported. the usual conversion of

#fides-overlay * {
  margin: 0
}

doesn't quite work because of CSS specificity rules I haven't quite wrapped my head around. It does apply that margin:0, but then our subsequent css doesn't overwrite it properly. I opted not to look into this too much, but we may want to consider adding a css reset to our fides demo page so that styling issues like this don't come up again (assuming we develop against the demo page)

* Use css reset for margins
* Move css import statement to Overlay component
@cypress
Copy link

cypress bot commented Jun 1, 2023

Passing run #2355 ↗︎

0 4 0 0 Flakiness 0

Details:

Merge 88d90f2 into c3b9e61...
Project: fides Commit: 75c43d6b38 ℹ️
Status: Passed Duration: 01:14 💡
Started: Jun 2, 2023 4:35 PM Ended: Jun 2, 2023 4:36 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 June 1, 2023 22:00
@allisonking allisonking mentioned this pull request Jun 1, 2023
10 tasks
Copy link
Contributor

@NevilleS NevilleS left a comment

Choose a reason for hiding this comment

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

This looks great to me, thanks. The only issue here is the use of Inter which isn't a standard browser font...

Assuming y'all figure out something for that, let's merge this in

Copy link
Contributor

@NevilleS NevilleS left a comment

Choose a reason for hiding this comment

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

Thanks 👍

@allisonking allisonking merged commit 2323f1d into feature/privacy-components Jun 2, 2023
@allisonking allisonking deleted the aking/3423/modal-css branch June 2, 2023 16:53
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