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

Use responsive container class #6

Merged
merged 1 commit into from
Jul 18, 2017
Merged

Use responsive container class #6

merged 1 commit into from
Jul 18, 2017

Conversation

shawnbot
Copy link
Contributor

This updates the container class from container (fixed-width) to container-lg px-3 (large with max-width + horizontal padding), as suggested in the styleguide. I thought that p-responsive might work here, but we might need to get a more recent version of Primer from npm to get that class, because it doesn't appear to be in the build on the most recently published site.

Also, I think that using container-md, with a max-width of 768px, might offer better legibility by reducing the line length:

container-lg container-md
image image

@shawnbot shawnbot requested review from benbalter and broccolini July 18, 2017 16:39
@benbalter
Copy link
Contributor

Wow this is great. Thanks for this. Had no idea.

I thought that p-responsive might work here, but we might need to get a more recent version of Primer from npm to get that class, because it doesn't appear to be in the build on the most recently published site.

If you run script/update it should update the version of Primer vendored (via NPM). I did so locally and got:

+ primer-base@1.1.5
+ primer-support@4.0.7
+ anchor-js@4.0.0
+ primer-markdown@3.3.13
+ primer-layout@1.0.5
+ primer-utilities@4.3.5

@shawnbot
Copy link
Contributor Author

Ah yeah, it looks like p-responsive was only added a month ago. We might want to pin the npm dependencies to at least a major version, which is something we've been discussing over in https://github.com/github/website-starter/issues/9.

@broccolini
Copy link

Yep, those are the latest versions and primer-utilities has p-responsive, however I'm not sure if that's the right style to use here. All our padding utilities have responsive variations, it depends how you want this to be laid out as to which padding you want to apply. Docs here: https://github.com/styleguide/css/styles/core/utilities/padding#responsive-padding

@shawnbot
Copy link
Contributor Author

Cool, I'll merge this then. We can sort out the versioning stuff later.

@shawnbot shawnbot merged commit 5f83e70 into master Jul 18, 2017
@shawnbot shawnbot deleted the responsive-layout branch July 18, 2017 17:27
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.

3 participants