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

Personal-Profile Katherine Fitzpatrick #25

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

Conversation

KatherineJF
Copy link

Personal Portfolio Site

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
Did you have to resolve any issues when running the HTML Validator? If so, what were they? Validator found tags that weren't closed which I resolved by closing or deleting tags.
Why is it important to consider and use semantic HTML? Provides a guide of page content used when the contents of the page are read aloud.
How did you decide to structure your CSS? Main styles.css with cascading styles for individual pages. Inherited styles were overwritten by local .css as needed.
What was the most challenging piece of this assignment? Figuring out how to manage multiple style sheets and layout was time consuming.
Describe one area that you gained more clarity on when completing this assignment Layout using flex and grid and style sheets.
Optional
Did you deploy to GitHub Pages? If so, what is the URL to your website?
Overall

@CheezItMan
Copy link

Personal Portfolio Site

What We're Looking For

Feature Feedback
Baseline
Appropriate Git Usage Reasonable number of commits and mostly good commit messages, some like format editist don't make much sense.
Answered comprehension questions Check, I'm glad you gained more clarity on multiple stylesheets and flex & grid.
Page fully loads Check
No broken links (regular or images) Check
Includes at least 4 pages and styling Check
HTML
Uses the high-level tags for organization: header, footer, main You should use more high-level sectioning tags which provide semantic meaning rather than using div elements with classes for the semantic meaning.
Appropriately using semantic tags: section, article, etc. You do overuse div a bit instead of using semantic section elements.
All images include alternate text Missing some alt attributes
CSS
Using class and ID names in style declarations Effective use of ids and classes
Style declarations are DRY Relatively DRY css content
Overall You do have some errors in your HTML content, and you should use more semantic HTML instead of div elements. I do love the media queries which adjust one page's layout for smaller screens. I also like how you made good use of grid and flex for the page layouts. Overall you hit the major learning goals of the project. Well done.

<link href="https://fonts.googleapis.com/css?family=Rock+Salt" rel="stylesheet">
<link href="styles/portfolio.css" rel="stylesheet">
<link href="styles/styles.css" rel="stylesheet">
<link href="styles/normalize.css" rel="normalize">

Choose a reason for hiding this comment

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

normalize is not a relationship, that should be stylesheet

</section>
<footer class = "column-layout">
<ul>
<li <a href="#" class="fa fa-facebook"></a></li>

Choose a reason for hiding this comment

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

The list items here are broken

<link href="styles/normalize.css" rel="normalize">
<link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/font-awesome/4.7.0/css/font-awesome.min.css">
</head>
<body1>

Choose a reason for hiding this comment

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

misspelled body element

</section>
</nav>
<!-- Header -->
<div class="header">

Choose a reason for hiding this comment

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

Why not use a header element?

<h1 id = "title">Hygee: Be Present with what you love</h1>
</div>
<!-- Photo Grid -->
<div class="container">

Choose a reason for hiding this comment

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

Why not use a main element?

</nav>

<!-- MAIN (Center website) -->
<div class="main">

Choose a reason for hiding this comment

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

Better to use a main element

<!-- Photo Grid -->
<div class="container">
<div class="photos">
<img src="images/IMG_0034.jpg" >

Choose a reason for hiding this comment

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

missing alt attributes

<nav class = "nav">
<section class ="column-layout">
<section class = "maincolumn">
<h3> <a href="index.html" target="_blank">Home</a></h3>

Choose a reason for hiding this comment

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

Making the target="_blank" for the links forces each link to open in a separate tag. Some people, myself included find this annoying.

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