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

Brandon White #344

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

Conversation

bwhite06
Copy link

No description provided.

Copy link

@Velsu Velsu left a comment

Choose a reason for hiding this comment

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

Great job on this sprint challenge. Most of tasks have been complete but there is a lot of room for improvement, especially with mobile layout. When You shrink the window, all the boxes get distorted, the top and bottom content's padding goes out of control and header nav is not responsive at all.

Best fix for this is to use flex direction for nav when max-width is not bigger than 400px and play around with padding and width of both content and boxes. Rest of the project on tablet and desktop modes look okay.

Overview:

  • Correctly place a viewport meta tag in the head of your index.html file that contains these values: content="width=device-width, initial-scale=1.0" (done)
  • In your CSS, Introduce font-size:62.5%; to the html element (done)
  • Convert all pixel font-sizes into rems and use rems for anything new you build (done)
  • Convert .container into a max-width responsive class based on it's fixed width's current size (done)
  • The header and jumbotron are missing from the code. Use the image named desktop-design.png to create a pixel perfect replacement. Please note, the desktop-design.png file is 1000px wide (partially done)
  • Update background colors for boxes based on screen width (done)
  • Adjust design based on design files (partially done)

Required revisions to pass the sprint:

  • Make header along with navigation responsive in both tablet and mobile mode based on design files

Possible improvements:

  • Change the header layout in mobile view to be column using flex
  • Adjust padding, width and margins of bottom and top content on mobile view
  • Adjust width of boxes in mobile view so they do not get shrinked too much
  • Missed one font-size that should be converted from px to rem

Copy link

@decagondev decagondev left a comment

Choose a reason for hiding this comment

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

Overview

Brandon, after careful review of your code base it seems that you have made the changes requested in the previous review. You have hit MVP and it seems that you have made the site responsive. in mobile and tablet view. it is pixel near enough as opposed to pixel perfect as the specs say but I think that it is within a margin we can allow. Take a look over your margin choices for the logo as they are a little off but your overall attempt and work since the request for change is a lot better. In a production / real world situation having a non pixel perfect to spec design would lead to a failure but as the progress you have made is well formed you have been given a 2 star for this project.

Conclusions

Brandon. your work is showing progress and if you can push yourself a little more you have the potential to become a good software engineer. I hope to see more from you in the future and see your progress as you get better in this field.

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