-
Notifications
You must be signed in to change notification settings - Fork 110
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
Sea Turtles - Georgia A #94
base: master
Are you sure you want to change the base?
Conversation
I see 3 commits for the work on this PR. If this is because some commits were squashed together, no worries, but if not, I strongly recommend making smaller, more frequent commits to checkpoint progress and give yourself known places to roll back to when you want to try things out. |
Under the comprehension question "Did you have to resolve any issues when running the HTML Validator? If so, what were they?" above, I see you replied N/A. Checking the HTML with a validator was listed as the first project requirement, so if you have not, I highly recommend going through that process, even if it doesn't surface issues. Without deploying your site, there are ways to verify the HTML, the validator linked in the README has a "Direct Input" option where you can copy/paste code to check it: https://validator.w3.org/#validate_by_input |
Noted regarding the git commits!
Regarding the HTML validator, I did use the W3C Web Validator extension in VS code to check my HTML files throughout the process of building my site. I read the comprehension question as only asking about any issues that came up. I received the "HTML file is valid!" pop up each time I ran the validator so I did not have the opportunity to work through any issues that came up. |
I can't leave comments on deleted files, so I can't batch them with the PR comments, sorry for the multiple notifications! I saw that the html files were pulled out of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice work, Georgia! I like the resizing on the header image and notepad background on the about page 😄 I've left some feedback and suggestions, please take a look when you can and reach out if there's anything I can clarify.
<section class="contact"> | ||
<h3>Contact</h3> | ||
<ul> | ||
<li><a href="mailto:[email protected]"><img alt="Email" src="assets/square.png"/></a></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice use of mailto
for email without javascript!
<meta http-equiv="X-UA-Compatible" content="IE=edge"> | ||
<meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||
<title>Georgia Adam</title> | ||
<link rel="shortcut icon" href="/assets/forest.png"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I like that the favicon ties in with the header image 😄
<nav> | ||
<ul> | ||
<li><a href="/index.html"> Home </a></li> | ||
<li><a href="/portfolio.html"> Portfolio </a></li> | ||
<li><a href="/about.html"> About </a></li> | ||
</ul> | ||
</nav> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the flow of the pages, I would consider the navigation as part of the header that starts off every page. Given that, I suggest moving these nav
blocks into the header
elements and styling as necessary so that the HTML better represents layout. This kind of organization can help folks with screen readers get a better understanding of the intended flow and content of a website.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok this makes sense! Good to know about the screen readers, I did not know that.
</h3> | ||
</section> | ||
|
||
<main class="grid_container"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A main
element is intended to represent the dominant content of the body, so I would consider including the section
above inside of this main
tag.
<h3> | ||
About me. | ||
</h3> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is completely a matter of preference, but since this is a short piece of text, it would be okay to do inline like you have in the header
above.
<div><a href="https://github.com/georgia-adam/task-list-api"><h2>Task List</h2></a><h3>Flask API</h3></div> | ||
<div><a href="https://github.com/georgia-adam/book-review-site"><h2>Book Review Site</h2></a><h3>HTML/CSS</h3></div> | ||
<div><a href="https://github.com/georgia-adam/swap-meet"><h2>Swap Meet</h2></a><h3>Python OOP</h3></div> | ||
<div><a href="https://github.com/georgia-adam/viewing-party"><h2>Viewing Party</h2></a><h3>Python</h3></div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
We should avoid
div
unless it is purely for styling. Since each of thesedivs
seems to have meaning through grouping together a block of content that represents a project, I would strongly suggest replacing thediv
tags with something likearticle
orsection
. -
This is conceptually a list of projects, so I would consider wrapping them in a list element and styling as needed to achieve the desired layout.
-
There are several tags on the same line, I think it would help the structure be clearer when reading the HTML if we broke these down into multiple lines:
<div>
<a href="https://github.com/georgia-adam/task-list-api">
<h2>Task List</h2>
</a>
<h3>Flask API</h3>
</div>
<h4>© 2022 Georgia Adam <br> | ||
Photo by <a href="https://unsplash.com/@aewild?utm_source=unsplash&utm_medium=referral&utm_content=creditCopyText">Amanda Frank</a> on <a href="https://unsplash.com/@aewild?utm_source=unsplash&utm_medium=referral&utm_content=creditCopyText">Unsplash</a> / | ||
<a href="https://www.flaticon.com/free-icons/tree" title="tree icons">Tree icons created by Freepik - Flaticon</a> / | ||
<a href="https://www.flaticon.com/free-icons/at" title="at icons">At icons created by Freepik - Flaticon</a> / | ||
<a href="https://www.flaticon.com/free-icons/linkedin" title="linkedin icons">Linkedin icons created by Freepik - Flaticon</a> | ||
</h4> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The h1-h6 tags are intended to represent the heading of a section. Here the main content of the footer is embedded in the h4
element. This can make it harder for screen reader users to understand and navigate the content. I would recommend using one or more
elements here and styling as needed to get the boldness and such you want from the font.
<li><a href="mailto:[email protected]"><img alt="Email" src="assets/square.png"/></a></li> | ||
<li><a href="https://www.linkedin.com/in/georgiaeadam/"><img alt="LinkedIn" src="assets/linkedin.png"/></a></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These lines are pretty long, I would consider breaking them up across multiple lines (which has the added benefit of making the nested structures clearer).
#bio { | ||
grid-column: 2; | ||
grid-row: 1 / 3; | ||
padding-left: 30px; | ||
padding-top: 12%; | ||
font-size: x-large; | ||
background: url(/assets/notepad-transparent-background-1.png); | ||
background-repeat: no-repeat; | ||
background-position: top; | ||
background-size: cover; | ||
border-left: 4px solid black; | ||
font-family: cursive; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This turned out really well, nice use of styling ^_^
@@ -0,0 +1,49 @@ | |||
* { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice organization for the styles to reduce repetition!
No worries about the notifications. For me it was a file structuring choice. The /pages added to the URL was bothering me. I know this may not necessarily be the best reason. I was able to create working href attributes both when the files were in the |
Personal Portfolio Site
Congratulations! You're submitting your assignment!
Comprehension Questions