cfpb / owning-a-home

⚠️ DEPRECATED ⚠️ Suite of tools and resources for homebuyers.
http://consumerfinance.gov/owning-a-home
Creative Commons Zero v1.0 Universal
94 stars 65 forks source link

Modify `@grid_wrapper-width` to match Capital Framework #723

Closed sebworks closed 7 years ago

sebworks commented 7 years ago

Modify @grid_wrapper-width to match Capital Framework

Changes

Testing

Notes

willbarton commented 7 years ago

@Scotchester can you get this reviewed?

Scotchester commented 7 years ago

This PR doesn't require cfpb/cfgov-sheer-templates#21, but you were right, @sebworks. The universal border-box rule does create a problem with this site.

If I run this by itself, the body width gets correctly updated, but the header width does not, because of the border-box rule. I took it out in my local environment, and the header now also appears at the correct width.

I looked through the entirety of each journey page, and removal of that rule didn't seem to have any negative effects, but we may want to ask an OAH dev if they know of any reasons that would be a bad idea. /cc @virginiacc @cfarm

sebworks commented 7 years ago

@Scotchester, changing styles in this repo shouldn't inadvertently affect the on-demand header and footer. The same way including the header and footer shouldn't overwrite your local styles. There should be some level of encapsulation happening ( a completely larger issue ).

Scotchester commented 7 years ago

@sebworks I certainly agree, but this is where we find ourselves at this time :)

sebworks commented 7 years ago

ok @Scotchester, what do you want to see happen? Should we just remove the border box styles? Without visual regression testing it's just guessing what happens at certain breakpoints.

Scotchester commented 7 years ago

I delayed answering this because I actually think I could set up a simple visual regression test for this pretty quickly, but couldn't get to it today. I will try to do that in the morning.

The only problem is that it won't be able to test what the content inside the expandables looks like.

Scotchester commented 7 years ago

Tests! Download the screenshots: box-sizing-vrt-results.zip

The screenshots in that file show a baseline of the current styling in this project (on this PR's branch, with box-sizing: border-box; still active), shots of the result of removing box-sizing: border-box; and diffs when those two things differed.

There are only diffs at the 320px and 1280px widths.

The only problem is that I've only tested the four journey pages. I know there are other pages in there that might be dicier, as they're more interactive. I'll test those tomorrow.

Scotchester commented 7 years ago

Visual regression test results for the four tool pages: oah-tools-box-sizing-vrt-results.zip

These tests, combined with my live perusal of each page after making the change, make me pretty confident that no major functional breakage will occur by removing the rule. There are a few minor visual glitches (e.g., form fields in Explore Rates butting up against each other instead of having space between them), but they don't make any content unreadable nor impair any functionality.

My suggestion would be to remove the box-sizing rule from the OAH codebase so that it comes back in line with the rest of our web products. I'd be glad to go through and mitigate the undesirable visual issues that creates, if it helps.

sebworks commented 7 years ago

@Scotchester, how would you feel about just limiting the scope of the box sizing so it doesn't affect the nav?

mthibos commented 7 years ago

@virginiacc @sosan just making sure you are looped into this conversation.

Scotchester commented 7 years ago

@sebworks I would certainly be open to that. Any ideas for how to easily achieve it?

sebworks commented 7 years ago

Can you just target primary-content?

Scotchester commented 7 years ago

I'll give it a try.

Scotchester commented 7 years ago

OK! I pushed a commit that successfully scopes the box-sizing rule to OAH's Sheerlike pages only. Visual regression testing confirms that the only "regressions" are overall page width adjustments; no interior messiness. With that, I think this PR is good to go.

@sebworks Give it a whirl and see what you think.

sebworks commented 7 years ago

Looks good @Scotchester. The visual regression stuff is cool too.