Galactimo / MI-449-SS18-740-css-spacing-and-layout-T7bJIb

0 stars 0 forks source link

Project Feedback #1

Open Galactimo opened 6 years ago

Galactimo commented 6 years ago

Create a professional-looking personal site or portfolio

@KatieMFritz Can you take a look at this? It's hosted here and meets the following criteria:

KatieMFritz commented 6 years ago

Ryan, this page is awesome! 😮 ✨ I love seeing all the projects you've done collected in one place, with screenshots and annotations - it's a perfect portfolio. 😻

You've done a really nice job with your code and layout. I just have a couple small layout challenges, and a few ways you can make the code a little more concise/readable.

Layout improvements

Use viewport meta tag for scaling

On my phone, I just see a tiny version of the desktop site, not a wrapped and zoomed in version. Check out this link about the viewport meta tag and add it to your site. It will make the mobile view much nicer!

Make images scale down

Your site responds well overall on narrow screens, but when I get below about 500px wide, the images get cut off. That's because you have a fixed width. Could you please try some different CSS so the images scale appropriately? You might need to use a combination of:

on the img and possibly a wrapper element to get this working as desired. Message me if you get stuck.


Code improvements

Use headings in order

Right now, you're using h2 for both the gallery headings and the individual item headings. You'll create a more semantic outline of your page if you use h3 for the individual item headings. You can still style them however you want with CSS.

Put picheader class directly on heading

Currently, you're using a div to wrap the heading for each gallery item, but it's not really needed - I believe you can apply your picheader class directly to the headings, without changing any of the styles, and save two lines of code for each item. Try it and see. 😄

Use figcaption instead of div for image descriptions

It's more semantic.

Move base font styles to body selector

You're repeating your font styles a lot. Try moving those shared styles (like font-family, color, and font-size) to your body CSS selector and just overriding them when needed. That will save you a lot of lines!

Check HTML indentation

Some 4-space indentation snuck into your nav. 👾


After you’ve made your changes and pushed them to GitHub and your hosted site, give it a once-over to make sure it looks right, then comment back here and I’ll take another look.

Thanks! :rocket:

Galactimo commented 6 years ago

@KatieMFritz The Scaling should now work correctly, and the syntax errors should now be fixed. I re-validated the CSS and HTML so it should be ready for another check.

-Ryan

KatieMFritz commented 6 years ago

Great job!

Just one more change now. On narrow screens and phones, your navbar wraps and cuts off the header:

phone view of site

I played around in the devtools a bit, and I think this should fix that:

Another note: You don't have to change this, but in general, many developers (and linters) avoid relying on element selectors for CSS. They can be useful, especially when you're styling basic elements like headings, but they also can limit your options for revision and it can make code a little more confusing. In your case, I'm thinking of the navbar class - it does different things depending on which element it's applied to, which is not a conventional way to use classes. Most CSS frameworks would put the navbar class on the nav or ul element, then use a nav-item or similar class for the lis, or select children of the navbar class with .navbar li. Does that make sense?

If that's an interesting concept to you, check out this blog post (it's a little old) about various systems for organizing CSS: https://mattstauffer.com/blog/organizing-css-oocss-smacss-and-bem/.

Galactimo commented 6 years ago

@KatieMFritz I was able to hopefully fix the problem by adding a @media css rule to when the screen is phone sized, I lower the amount of width that the padding between the navbar links take up. I tested this on almost every phone model that the inspector has and it worked on all of them. I also did the things you requested to try and fix it, but It was still happening until I lowered the padding. As for the other note, I will keep that in mind for structuring future projects.

Thank you,

Ryan Honey

KatieMFritz commented 6 years ago

That works! Great job iterating on this. :shipit: 🎉

Would it be okay to use your hosted site to show off some of the projects from the course? We wouldn't publish it, just show it to other department faculty and possibly other potential clients for our software/curriculum. If you'd prefer we didn't, not problem. Please let me know either way. 😁

Galactimo commented 6 years ago

@KatieMFritz Please by all means use it for whatever you want/need. Its the least I can do for all I've learned in this class.

Thank you so much,

Ryan