Skip to content

LG-3879: Use design system grid for account header#5953

Merged
aduth merged 11 commits intomainfrom
aduth-lg-3879-nav-auth-grid
Feb 24, 2022
Merged

LG-3879: Use design system grid for account header#5953
aduth merged 11 commits intomainfrom
aduth-lg-3879-nav-auth-grid

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Feb 15, 2022

Why: To simplify markup and to standardize on design system utilities.

For design review (cc @anniehirshman-gsa):

  • Can we confirm the desired container width of the header portion? In live and in Figma, it's 908px. The design system default container width is 960px. I've added 3rem padding on desktop to split the difference a bit to 928px. Do we want to add more to get closer to what we had (3.5rem = 912px, 4rem = 896px)? Would we consider aligning to the design system?

Screenshots:

Size Before After
Mobile localhost_3000_account(iPhone XR) (1) localhost_3000_account(iPhone XR)
Tablet localhost_3000_account(iPad Air) (1) localhost_3000_account(iPad Air)
Desktop localhost_3000_account (1) localhost_3000_account

@anniehirshman-gsa
Copy link
Contributor

The header portion is just the Login.gov logo and Welcome/Sign out, right? If so, that change looks fine to me. IIRC I mostly mocked up the Figma based on the live implementation so I don't think we need to adhere to closely to that, and can switch to design system defaults if it doesn't break anything.

Personally I would align the logo to the left edge of the sidebar, and the Welcome/Sign out button and text to the right edge of the main - so 780 px total? And not have it break to the mobile header at such a wide width. But understand if that's outside the scope of this work.

@aduth
Copy link
Contributor Author

aduth commented Feb 15, 2022

Personally I would align the logo to the left edge of the sidebar, and the Welcome/Sign out button and text to the right edge of the main - so 780 px total?

This might simplify things actually, since we could set a single consistent grid container width.

@aduth aduth marked this pull request as draft February 16, 2022 13:01
@aduth
Copy link
Contributor Author

aduth commented Feb 16, 2022

Personally I would align the logo to the left edge of the sidebar, and the Welcome/Sign out button and text to the right edge of the main - so 780 px total? And not have it break to the mobile header at such a wide width. But understand if that's outside the scope of this work.

Looking into this, the configurability of this in the design system is through the $theme-grid-container-max-width variable, which can be set to one of the spacing units. By default, it's set to 1024. Combined with $theme-site-margins-width of 2rem padding, this makes the default container content width 960.

To get to a number closer to what we have currently for the main content area, we can change the max-width to tablet-lg, which is 880 by default, reduced to 816 content area with default padding.

@anniehirshman-gsa What would you think about a content area of 816? We could tinker with increasing $theme-site-margins-width to get more padding (smaller content area), but this can have some odd side-effects, like increasing vertical padding on section content. It's also worth noting that the IdP's account page content is definitely not implemented with a standard grid, instead still using parts of BassCSS combined with padding utilities.

Preview at 816:

image

@anniehirshman-gsa
Copy link
Contributor

anniehirshman-gsa commented Feb 16, 2022

To get to a number closer to what we have currently for the main content area, we can change the max-width to tablet-lg, which is 880 by default, reduced to 816 content area with default padding.

This sounds good to me and the screenshot looks good - though would you be able to push it to your personal env so I can check the responsiveness/breakpoint?

ETA: The illustration/intro text block is off-center, but I think it was like that before? Would be great to fix either now or when other parts of the BassCSS grid are removed from the Account page.

@aduth
Copy link
Contributor Author

aduth commented Feb 16, 2022

ETA: The illustration/intro text block is off-center, but I think it was like that before? Would be great to fix either now or when other parts of the BassCSS grid are removed from the Account page.

Good point! I recall asking about this when it was first implemented, and it was semi-intentional, though maybe undesirable at least with the updated grid sizing. I'll see about centering it.

@aduth
Copy link
Contributor Author

aduth commented Feb 16, 2022

This sounds good to me and the screenshot looks good - though would you be able to push it to your personal env so I can check the responsiveness/breakpoint?

ETA: The illustration/intro text block is off-center, but I think it was like that before? Would be great to fix either now or when other parts of the BassCSS grid are removed from the Account page.

Updated grid in 146c5bd and centered the illustration intro block in 430d73f, both of which are available to take for a spin now in my sandbox.

Copy link
Contributor

@anniehirshman-gsa anniehirshman-gsa left a comment

Choose a reason for hiding this comment

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

Account page breakpoints and tablet view look awesome now! 👏

Only thing would be the visual issue that Doug found - what happens when both "Verified" and "Unphishable" badges appear. I would just move both them below the Your Account h1 at the tablet breakpoint (or all breakpoints) to keep it simple, but open to other ideas.

Screen Shot 2022-02-18 at 12 58 03 PM

@anniehirshman-gsa
Copy link
Contributor

Oh also - there's this extra margin that I think can go away or be reduced? But lmk if you think that would cause issues (on mobile or elsewhere)


Screen Shot 2022-02-18 at 1 10 53 PM

@aduth
Copy link
Contributor Author

aduth commented Feb 18, 2022

Oh also - there's this extra margin that I think can go away or be reduced? But lmk if you think that would cause issues (on mobile or elsewhere)

The branch in my sandbox is a bit out of sync with main, but this was fixed with #5954.

@aduth
Copy link
Contributor Author

aduth commented Feb 18, 2022

Only thing would be the visual issue that Doug found - what happens when both "Verified" and "Unphishable" badges appear. I would just move both them below the Your Account h1 at the tablet breakpoint (or all breakpoints) to keep it simple, but open to other ideas.

Sure, I updated collapsing behavior so that it'll move under if it runs out of space. It's also on its way to my personal sandbox, along with the fix of #5954.

Width Before After
1024 localhost_3000_account (3) localhost_3000_account
800 localhost_3000_account (2) localhost_3000_account (1)

@aduth aduth marked this pull request as ready for review February 18, 2022 19:06
Copy link
Contributor

@anniehirshman-gsa anniehirshman-gsa left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

@aduth aduth requested review from peggles2 and solipet February 23, 2022 13:09
@aduth aduth force-pushed the aduth-lg-3879-nav-auth-grid branch 2 times, most recently from 7322497 to 3bf0378 Compare February 23, 2022 13:39
Copy link
Contributor

@solipet solipet left a comment

Choose a reason for hiding this comment

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

LGTM!

@aduth aduth force-pushed the aduth-lg-3879-nav-auth-grid branch from efec055 to 16a55ca Compare February 24, 2022 13:18
'mobile-lg': true,
'tablet': true,
'tablet-lg': false,
'desktop': false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Disabling this will have a nice performance impact, reducing the size of the generated stylesheet by a fair bit, since each of these breakpoints generates a lot of utility styles.

Overall, this pull request reduces the size of the stylesheet by ~11% (62.2kb to 55.4kb gzipped).

@aduth aduth merged commit 2100b7b into main Feb 24, 2022
@aduth aduth deleted the aduth-lg-3879-nav-auth-grid branch February 24, 2022 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants