-
Notifications
You must be signed in to change notification settings - Fork 78
Make Code Corps responsive. Add bourbon media query breakpoints. #418
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
Conversation
f5e3f47 to
9932ffe
Compare
|
Overall, looks great. Some notes.. https://cloud.githubusercontent.com/assets/13618860/18238713/9766ea4a-72f4-11e6-8a71-38e481faa629.png <-- would look slicker if the icons were placed inside the input fields, right aligned (kinda looks awkward when its just a check below the input). But don't think its a big deal. https://cloud.githubusercontent.com/assets/13618860/18229466/4a0d934a-722f-11e6-9707-453d20aaeb66.png https://cloud.githubusercontent.com/assets/13618860/18229470/54175f56-722f-11e6-9c8c-1c79d813e2c6.png <-- I kinda like that separator line in the nav (no other screenshots have them). Maybe we can make that part of mobile design? |
|
@olegko thanks for the feedback! The way the skills naturally wrap from desktop to mobile made the 2nd row have only two items. I left it that way because it's symmetrical, but I can play around with it. Nice thought putting the icons inside the input fields. It's gonna take some work to get that to display correctly, but I think it might be worth it. The text boxes that are getting cut off are an artifact of a bad screenshot from an older commit. We have since added horizontal padding to the body, and it is not issue. The separator line is actually the top border of the code corps project card. The side borders are getting cut off for the same reason I mentioned above. I agree that it kinda looks nice though, and I think we should A/B adding a separator to the nav. |
97dc643 to
45bf2f7
Compare
|
I fixed the wrapping issue with .demo-skills section of index.hbs thanks to feedback from @olegko, and I've updated the screenshot to reflect the change. Also, I edited a screenshot of the signup screen to make the success and error icons appear inside the text fields. Implementing this in css will take quite a bit of work, so I wanted to make sure we all agree this is a desired change before I spend too much time trying to figure it out. Here is the edited screenshot: |
|
@appleJax I'm not a huge fan of that change. I don't think we lose anything by having the icons appear below, alongside the text. I would heartily advocate for less code if the UI isn't meaningfully altered. |
|
@joshsmith ok, that's a relief. Do you know who all the design folks are on github? I only know of @ivy-g. I think they should be part of the conversation on this PR. |
|
I'm not sure who everyone on here is who have been involved in design. It's primarily been myself. I need to loop back on this after I get everything wrapped up on the Phoenix deployment side. |
|
@appleJax much prefer not merging |
|
I'm actually not sure how that last commit got there. I merged develop into my branch yesterday before I fixed the wrapping issue, but the last thing I did was rebase, and that was before I pushed anything or commented on here about the change. |
|
@appleJax you or someone else pushed the button in the GitHub UI. That button is bane of my existence. |
|
@joshsmith oh damn, I'm on my phone right now, so I'm not sure what button you're talking about, but it was probably me, especially if it's near the assignee dropdown. |
|
Oh ok, I found the button, lol. Will try to resist the urge to press it... |
|
Wow, this is awesome work! I just had some opinions/suggestions, but they're not exactly relevant here and so could be next steps if there's interest.
|
|
@appleJax happy to help resolve the conflicts. let me know if I can help! |
|
@marineb thanks, I can take care of it. I was under the impression that this PR was on hold, so I was kinda waiting for us to get around to it rather than keeping it up-to-date through every change in the codebase. Now is a good time for an update though, it's been a while. |
|
@appleJax I have renewed urgency to unblock you. ❤️ |
7dff510 to
3167e98
Compare
|
@appleJax looks like this got caught at a weird time that builds weren't happening. Would you mind a quick rebase to try and restart the build? Also, feel free to invite me to your repo so I don't have to bug you about something that simple. |
3167e98 to
b8ce86a
Compare
|
@appleJax I think that builds are working again. Can you try a fresh push? |
b8ce86a to
bb9ab2f
Compare
|
Yeah, no prob. I will just update develop and branch from there.
…On Feb 9, 2017 11:55 PM, "Josh Smith" ***@***.***> wrote:
@appleJax <https://github.com/appleJax> there are some new conflicts due
to upstream merges from #846
<#846>. We can go
through them if you'd like, although I know you were trying to use this
mostly as frame of reference for a new PR, anyway.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#418 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AM_OrG66bpc8N0ccSEe72P4P34hjkL15ks5rbBgEgaJpZM4JxOSK>
.
|
|
Closing in favor of another PR. @appleJax has fork for ref. |





















What's in this PR?
I took a stab at making the site responsive.
Basically, I used bourbon media queries and changed the
@include span-columns()attribute of certain elements to make them fit better on smaller screens. The first thing I did was create _breakpoints.scss to house the media query breakpoints.I used the same breakpoints Bootstrap uses, except with max-widths instead of min-widths since Code Corps is already desktop-first. That means if you set styles for a medium-sized screen, the styles will apply to everything below that size unless you override them with another media query for a smaller screen. Large screens (1200px or greater) are the default and don't need a media query. The breakpoints and variable names are as follows:
Here's a bourbon-flavored media query:
This is by no means a final solution. Just wanted to put this out on the table as an option and get some dialogue going.