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

Queues - Erica J. Case - Static-Site #24

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

Conversation

EricaJCasePhD
Copy link

@EricaJCasePhD EricaJCasePhD commented Mar 20, 2017

Static 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? Yes...an extra li tag that got copied and pasted repeatedly.
Why is it important to consider and use semantic HTML? To help others understand your code and for screen readers.
How did you decide to structure your CSS? One file with comments.
What was the most challenging piece of this assignment? Positioning. I took out a lot of elements because they looked terrible and I couldn't make them look better.
Describe one area that you gained more clarity on when completing this assignment Some selectors.

@PilgrimMemoirs
Copy link

Static Site

What We're Looking For

Feature Feedback
Baseline
Appropriate Git Usage ❗️ Need to commit more often, with more specific messages on what was added, changed or removed
Answered comprehension questions Well Done - if something you're trying to accomplish isn't working, instead of deleting comment it out. That way we can always look over it and give some feedback on how you could have made it work :)
Page fully loads Well Done
No broken links (regular or images) Well Done
Includes at least 4 pages and styling Well Done
HTML
Uses the high-level tags for organization: header, footer, main Well Done
Appropriately using semantic tags: section, article, etc. Well Done
All images include alternate text ❗️ No Images used
CSS
Using class and ID names in style declarations Well Done
Style declarations are DRY Well Done - Nice use of multiple selectors for rulesets
Overall
Layout Great work at incorporating layout techniques we learned into a different layout. It looks great and is easy to navigate!
Measurements Nice use of using responsive measurements where appropriate!
Organization Code is well organized and very easy to read!

Copy link

@lbueing lbueing left a comment

Choose a reason for hiding this comment

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

Hi Erica!
Overall, Great job! You'll notice that I added a lot of comments that are 'nit-picky'. Most of them are in regards to typos/spacing. While spacing doesn't affect the outcome of your code, it's a stylistic thing that will get noted in industry code reviews. Also, for many of the spacing instances, I only commented on the first or second time it happened, but you may want to scan your code to note where other occurrences are.

You did an excellent job of using semantic tagging. Your layout was neat and tidy, very easy to read. It's clear you put a lot of work into this!

<h2 class="left">Ecologist</h2>
<ul class="left">
<li class=>Plant community dynamics</li>
<li >Invasive species</li,>
Copy link

Choose a reason for hiding this comment

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

line 33: nit - Extra space and comma after li's.

</head>
<body>
<header>
<h2><i class="material-icons" style="font-size:36px">home</i></h2>
Copy link

Choose a reason for hiding this comment

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

line 15: Best practice to keep as much of your styling in you css sheet.

<title>Portfolio Project</title>
<link rel="styles/normalize.css" href="icono.min.css">
<link rel="stylesheet" href="styles/main.css">
<link href="https://fonts.googleapis.com/icon?family=Material+Icons" rel="stylesheet">
Copy link

Choose a reason for hiding this comment

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

line 8: nitpick - It's nice to have all your links in the same general format. rel then href on each

<li>Rare species conservation</li>
</ul>
</div>
<i id = "recycle" class="fa fa-recycle"></i>
Copy link

Choose a reason for hiding this comment

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

line37: What is this line for?
nit - id="recycle" remove extra spaces

</ul>
</div>
<i id = "recycle" class="fa fa-recycle"></i>
<div, id="Programmer">
Copy link

Choose a reason for hiding this comment

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

line 38: , instead of >

<li >Invasive species</li,>
<li>Rare species conservation</li>
</ul>
</div>
Copy link

Choose a reason for hiding this comment

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

line 34-36: Check your indentation.

<header>
<a href="index.html" class="home"> <i class="fa">&#xf1b8;</i></a></li>
<h1 class="erica">Erica Case: PROJECTS</h1>
<a href="projects.html" class="portfolio"> <i class="fa">&#xf0f2;</i></a></li>
Copy link

Choose a reason for hiding this comment

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

line 14: nit - extra space after <a
line 14 and 16: nit - no space needed between the a tag and the i tag


<footer>
<a href="blog.html" class="blog"> <i class="fa">&#xf02d;</i></a></li>
<a href="me.html" class="me" > <i class="fa" >&#xf1ae;</i></a></li>
Copy link

Choose a reason for hiding this comment

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

line 69-70: These font awesome things are really cool! Nice find!

</main>



Copy link

Choose a reason for hiding this comment

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

No need to leave more than one extra line in between.

<h3>Ruby Code</h3>
<aside>
<h4>Development</h4>
<i class="fa"> &#xf175;</i>
Copy link

Choose a reason for hiding this comment

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

line 55: Spacing before text

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.

4 participants