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

Responsive-Design-2 #876

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Christian-Ford
Copy link

No description provided.

Copy link

@lolax lolax left a comment

Choose a reason for hiding this comment

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

Great work on this project, Christian. You were thorough with your switch to a fully responsive design. My only recommendation is that you avoid using percentages for height and top/bottom padding & margins because we don't want to shrink everything on small screen dimension, just make it more minimal and longer/narrower. Vertical scrolling is expected so it's okay for elements to be a little tall, horizontal scrolling is what we want to eliminate. Nice work! 💯

justify-content: center;
width: 70%;
height: 8%;
Copy link

Choose a reason for hiding this comment

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

Avoid using percentages for heights and top/bottom margins & padding. Vertical scroll bars are perfectly fine and we don't want content to get super squished if someone's window height is small, they will just have to scroll more downwards.

.button {
border: 1px solid black;
width: 30%;
Copy link

Choose a reason for hiding this comment

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

Nice job with the percentages for widths

/* =========== Mobile View ===========*/

@media (max-width: 500px) {
Copy link

Choose a reason for hiding this comment

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

Good job with your breakpoints 💯

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.

2 participants