Phase 2 CSS improvements for consent banner and overlay #4334
Conversation
Passing run #4838 ↗︎
Details:
Review all test suite changes for PR #4334 ↗︎ |
||||||||||||||||||||
|
Hey @allisonking, heads up, I just made a tiny fix to the drop-down carets, they were being pushed out of the div if the corresponding text was too wide 279a9d1 |
|
Got it, starting review now! |
|
small thing—could we add a lil bit of margin right to the iab tcf tag in small viewports? it seems to run into the legint toggle a bit |
| }) => { | ||
| const [isMobile, setIsMobile] = useState(window.innerWidth < 768); |
There was a problem hiding this comment.
can we put 768 as a constant somewhere?
There was a problem hiding this comment.
this whole state + useEffect could go into hooks.ts to keep this component cleaner
|
|
||
| useEffect(() => { | ||
| const handleResize = () => { | ||
| setIsMobile(window.innerWidth < 768); |
There was a problem hiding this comment.
a lil bit of repeated code here—could pull out into a func? or at least make the 768 a reused constant
| renderModalFooter={() => | ||
| renderModalFooter({ | ||
| onClose: handleCloseModal, | ||
| isMobile: false, |
| min-height: 40px; | ||
| align-items: center; | ||
|
|
||
| & svg { |
There was a problem hiding this comment.
I think this kind of nesting doesn't work in firefox 😢
see firefox on left, chrome on right
There was a problem hiding this comment.
I'm on firefox 116 (which is a little out of date) which doesn't support it, seems to only be a recent thing https://caniuse.com/css-nesting
| /> | ||
| ) : null | ||
| } | ||
| renderModalContent={({ onClose }) => ( | ||
| renderModalContent={() => ( |
There was a problem hiding this comment.
if this no longer needs an argument, I think it'd be cleaner to revert it back to a component, i.e.
modalContent={
<div>
...
</div>
}or even to let this be children
Nice catch! The tag should be left-aligned even for the small viewports |
|
Thanks for the review @allisonking 🙏 I addressed the visual issues but in the interest of time, I'm moving the code-cleanup recommendations to the next sprint. I'll address them when I come back from break. |
Closes #4274
Description Of Changes
This PR focused on:
I tried to make the layout changes with just CSS but there were enough differences between the desktop and mobile layouts that I think it made sense to have an
isMobileprop to conditionally render the components instead of trying to nest them or modify them with CSS.Code Changes
isMobilestate to theConsentBanner.tsxConsentModal.tsxto usefides-modal-header,fides-modal-body, andfides-modal-footerSteps to Confirm
Pre-Merge Checklist
CHANGELOG.md