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

Add XL-friendly layout. #624

Merged
merged 7 commits into from Dec 10, 2019
Merged

Add XL-friendly layout. #624

merged 7 commits into from Dec 10, 2019

Conversation

andrew-healey
Copy link
Contributor

Description

This includes centering content Medium-style and keeping the footer always at least at the bottom of the page. See #207 for information on exactly what this implements.
Uses flexbox and margin:0px auto; to center things. Uses max-width to set the maximum width of the main content. This creates an effect similar to what Medium uses. The main content is set to be min-height:100vh;, and is structured with Flexbox around a) a div holding the main content and b) the footer. The div has property flex-grow:1;, allowing it to grow to satisfy the min-height:100vh; requirement. This places the footer aligned at the bottom.

I used @media queries to limit width of items only when the viewport width is greater than 1280px. The max content width limit under XL circumstances is 1140px.

Related Issue

#207

Screenshots (if appropriate):

1244px viewport width (Original site, my changes):

Original site, width <1280px
Changed site, same width

>1280px viewport width (Original site, my changes):

Original site, width >1280px

Changed site, same width

keeping the footer always at least at the bottom of the page.
Copy link
Contributor

@samajammin samajammin 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 the submission @Sesamestrong!

This looks great except for the sidebar on various pages, e.g:
Image 2019-12-04 at 9 20 48 PM

As mentioned in the issue, we want all content to be constrained within this max-width container. Please make the necessary updates to gain approval. You'll likely want to change the sidebar styles from the fixed positioning.

@andrew-healey
Copy link
Contributor Author

Will do!

@andrew-healey
Copy link
Contributor Author

andrew-healey commented Dec 6, 2019

I have added a change. Here is the new behavior (only changed on XL screens):

image

@samajammin
Copy link
Contributor

Hey @Sesamestrong thanks for continuing work on this.

I'm seeing this on the preview homepage on my laptop (1440px width):

deploy-preview-624--ethereumorg netlify com_

Looks like one of your changes is now forcing this content to align left. Same with the /build page.

Please resolve that.

@andrew-healey
Copy link
Contributor Author

I actually have a question about this. The reason I aligned it left was so that there could be space-efficient division between content and the sidebar. Should I keep it left-aligned when a sidebar is present, or should I center the main content in all cases and just limit the max-width in order to make it not overlap with the sidebar?

@andrew-healey
Copy link
Contributor Author

Option 1: Maintain centering, reduce width when sidebar is present

image

  1. Remove centering when a sidebar is present

image

@andrew-healey
Copy link
Contributor Author

I went with option 2.

@andrew-healey
Copy link
Contributor Author

I believe that it has all features that you want now.

Copy link
Contributor

@samajammin samajammin 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 to me! Thanks for the revisions. Made a couple comments - I'll merge now then make those changes.

@@ -154,4 +154,8 @@ function resolveOpenGroupIndex (route, items) {
.sidebar
display block !important
transform translateX(0)

@media only screen and (min-width:1280px)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add this width as a variable in config.styl

Copy link
Contributor

Choose a reason for hiding this comment

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

We do: $breakL

max-width 1140px
#wrapper.has-sidebar main
margin-left 10px
max-width "calc(1140px - %s - 10em)" % $sidebarWidth
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add this width as a variable in config.styl

@samajammin samajammin merged commit 9b4191c into ethereum:master Dec 10, 2019
@andrew-healey
Copy link
Contributor Author

@samajammin I have made a commit on my repo with the changes that you have requested. Should I make a new PR or is it possible to re-merge this one?

@samajammin
Copy link
Contributor

Thanks, @Sesamestrong.

I've reverted this PR in #634 - go ahead & make a new PR with all your changes (and please make the PR against the dev branch, not master). You can learn more about our deployment process here: https://github.com/ethereum/ethereum-org-website#deployment-lifecycle

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.

None yet

2 participants