-
Notifications
You must be signed in to change notification settings - Fork 1
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
feature: Standardize text container styles #26
Conversation
} | ||
|
||
.centerText { | ||
.content { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The .content
class is currently the same for each page, but I'm opting not to condense these as that may not be true for long. If we find this is common for other pages too we can abstract it then (WET - Write Everything Twice).
.landingBackground { | ||
padding: 1px; /* hack to prevent margin collapse */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is... dumb. There are better way to do this, but the background image doesn't really work with the navbar (because the navbar takes up height causing the image to go past the bottom of the page) and it's a larger overhaul to address that. Will revisit this problem later; we may end up changing how this background image is used anyways.
margin: 0 auto; | ||
max-width: 1000px; | ||
.image { | ||
margin: auto; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Little bit of CSS/DOM simplification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thank you for doing this! Only left one comment but otherwise LGTM
src/app/globals.css
Outdated
@@ -13,11 +13,26 @@ | |||
} | |||
|
|||
body { | |||
width: 100vw; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Question]: On Firefox this works like a charm but on Chrome vertical scrollbars factor into the total view-width of the page, forcing a horizontal scrollbar at the bottom :'(. Is there any way we could omit the width
property to prevent this or set it to width: 100%?
(for both this and landingBackground
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like Firefox and Safari are correct according to the spec, Chrome is off in wonderland. Sigh, this is easily the most painful part of web development. Good catch though; width: 100%
should be fine in our use case (but is not generally equivalent).
This standardizes text container styles (h1/h2, p, and a generic content container) and moves the shared styling into
globals.css
. Additionally, this fixes the gradient background to ensure it uses the full page and isn't affected by scroll (fixed
). See self-review for a few notable call-outs.Page changes (Before/After) - mostly the same, just a bit better with things like padding and consistency. Note that the landing background height isn't correct anymore following navbar changes - opting to consider this out of scope for now (and this might be changed shortly anyways).