Accelerate - Trish G. #9
Conversation
There was a problem hiding this comment.
Nice job!
The index and about pages are looking good. You have a reserved style across your site that works well, and would be amenable to all kinds of further development. Though I'd keep it subtle. The little touch of reducing the opacity of the main landing image is a nice touch and really helps soften the image.
There were a few minor validation issues, so be sure to check your source files against an HTML validator. This can be really useful for accessibility-focused attributes like the img alt attribute, which we might not otherwise notice is missing.
Your style is looking really clean, so just be careful as you add more content to make sure it stays that way, keeping shared style in style.css where possible and only overriding in per-page styles where necessary.
It would be great to see some of the repeated elements we'd expect to find in the portfolio or blog pages, so think a little bit about how you'd like those to look and flow.
Overall, you're off to a good start.
| <main> | ||
| <h1>About</h1> | ||
| <section class="container"> | ||
| <img id="about_pic" src="images/trish_photo.jpg"> |
There was a problem hiding this comment.
Be sure to add alt attributes to your images. Running your markup through a validator can help catch this sort of issue.
| @@ -0,0 +1,41 @@ | |||
| <!DOCTYPE html> | |||
| <html> | |||
There was a problem hiding this comment.
It's recommended to include a lang attribute on the html tag. A validator can help catch this by reporting warnings.
| </header> | ||
| <main> | ||
| <h1>About</h1> | ||
| <section class="container"> |
There was a problem hiding this comment.
Since this section really seems to exist mostly as a place to attach style (for the flex layout), a div might be a more appropriate choice. section elements should usually contain a heading of some kind, and should be thought of a something that might make sense to list in a table of contents for the page.
| <link href="styles/style.css" rel="stylesheet"> | ||
| <link href="styles/index.css" rel="stylesheet"> |
There was a problem hiding this comment.
Nice use of a site-wide style sheet and a per-page stylesheet.
| <nav class="navbar"> | ||
| <ul class="nav-links"> | ||
| <li class="nav-item"><a href="index.html">Home</a></li> |
There was a problem hiding this comment.
Having a navbar class on the nav element feels redundant. And for the child elements, rather than applying a special nav-related class, you could write your selectors to look for ul or li elements that are descendants of a nav element.
| font-size: 30px; | ||
| } | ||
|
|
||
| .nav-links { |
There was a problem hiding this comment.
Consider using nav > ul as the selector.
| display: flex; | ||
| } | ||
|
|
||
| .nav-item a { |
There was a problem hiding this comment.
Consider using something like nav a as the selector.
|
|
||
| .navbar { | ||
| background-color: #22BDA0; | ||
| display: flex; |
There was a problem hiding this comment.
Nice to see you going all-in on flex in your layout!
| } | ||
|
|
||
| .nav-links { | ||
| list-style: none; |
There was a problem hiding this comment.
Great way to take the semantic notion of a list of links and remove the list default styling.
| @@ -0,0 +1,11 @@ | |||
| img { | |||
There was a problem hiding this comment.
Great to see how small the per-page stylesheet is. We want to have as much common styling as possible, and have the minimum necessary to override layout for particular pages.
removes feedback
Not completely finished. I think my actual portfolio site will be built with something else. Turning this in despite it lacking portfolio section, and hobby or blog space.