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

Design Review - style(App): Fixes having the unnecessary whitespace below the FooterCfGov #369

Merged
merged 9 commits into from
Apr 8, 2024

Conversation

shindigira
Copy link
Contributor

@shindigira shindigira commented Apr 2, 2024

closes #365

Changes

  • style(App): Fixes having the unnecessary whitespace below the FooterCfGov

How to test this PR

  1. yarn install to get the latest DSR update
  2. Go to any page where FooterCfGov is used (basically all)
  3. Use mobile view and swipe up at the scroll end OR zoom out (CMD + - multiple times)
  4. See how the footer's grey part still extends to the bottom

Screenshots BEFORE the fix

Screenshot 2024-04-02 at 12 29 43 PM
Screenshot 2024-04-02 at 12 31 41 PM

Screenshot - Fix B (View zoomed out) - Maximizing the footer

Screenshot 2024-04-02 at 10 15 16 PM

…er while zoomed out or when swiping on mobile
@shindigira shindigira changed the title style(App): Fixes having the unnecessary whitespace below the FooterCfGov Design Review - style(App): Fixes having the unnecessary whitespace below the FooterCfGov Apr 2, 2024
Copy link
Contributor

@billhimmelsbach billhimmelsbach left a comment

Choose a reason for hiding this comment

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

Heyo! I think putting the background on a div restricted to the size of this div with h-dvh has some weird effects on pages that are larger than the viewport:
Screenshot 2024-04-02 at 6 27 47 PM

Digging through git, it looks like this h-dvh was set to help with making loading screens take up the viewport, but if you put this on a div that was set to auto, this approach works. 🎉

@shindigira
Copy link
Contributor Author

shindigira commented Apr 3, 2024

@billhimmelsbach @natalia-fitzgerald

Found a flex box-based solution for the overall screen layout which seems to work best. Right now we need to choose between Fix A and Fix B (shown at the top).

Also, requires cfpb/design-system-react#333 merged in first to see the change.

@natalia-fitzgerald
Copy link

@shindigira
I prefer Fix B - extending the gray background - Maximizing the footer.

function FooterCfGovWrapper(): JSX.Element {
// TODO: Choose between:
// Maximizing the midsection white space with 'mt-auto' or maximizing the gray space in the footer with 'flex-1
return <FooterCfGov className='flex-1' />;
Copy link
Contributor Author

@shindigira shindigira Apr 4, 2024

Choose a reason for hiding this comment

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

If you want to see the change before cfpb/design-system-react#333 is merged in, can manually edit this in the browser's devtools.

Choose a reason for hiding this comment

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

@shindigira - How can I see it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For you, we should put it on AWS.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is up for your review on the preview site @natalia-fitzgerald 👍

shindigira added a commit to cfpb/design-system-react that referenced this pull request Apr 4, 2024
…to pass to the inner Footer (#333)

closes #332 

Allows for custom styling which is needed in
cfpb/sbl-frontend#369
Copy link

@natalia-fitzgerald natalia-fitzgerald 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 addressing this. Looks good @shindigira!

Copy link
Contributor

@billhimmelsbach billhimmelsbach left a comment

Choose a reason for hiding this comment

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

Looks good! 🎉

@shindigira shindigira merged commit 3d6f145 into main Apr 8, 2024
2 checks passed
@billhimmelsbach billhimmelsbach deleted the 365-footer-below-too-much-white-space-2 branch May 31, 2024 14:45
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.

[Design Review] [FooterCfGov] Too much white space below footer when zoomed out
3 participants