-
Notifications
You must be signed in to change notification settings - Fork 36
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 - Kayla Kubicke - Static-Site #47
base: master
Are you sure you want to change the base?
Conversation
Static SiteWhat We're Looking For
Great work overall. Site is clean and easy to navigate, and HTML is well-structured and easy to read. I like how well-commented your CSS is - very easy to tell what's affecting what. As your CSS continues to grow more complex, it might make sense to split out the CSS for different pages into different files, so that each page can link only the styles it needs. You'll also probably want to move the CSS files into their own folder. So the project setup might look like
and then in the individual HTML files: <!-- hobby-blog.html -->
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8">
<title>Hobby Blog</title>
<link href="stylesheets/style.css" rel="stylesheet">
<link href="stylesheets/hobby-blog.css" rel="stylesheet">
<!-- ... --> |
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.
Hey, Kayla! Overall, good work! I made a tonnnnnn of comments, especially in the CSS. If they feel overwhelming, feel free to disregard; I know that you've only had one week of CSS!
Make sure to remove unnecessary comments when you submit pull requests (I didn't learn this until I started my internship). If you were commenting out code because you were trying out a few different things but didn't wan to lose what you'd written, make sure to clean it all up before PRing.
Also, read up on great commit and pull messages: https://chris.beams.io/posts/git-commit/ The earlier you get into the habit of high caliber commit messages, the better a teammate you'll be when it comes to working on projects with others!
Again, nice work! Let me know if you have any questions about any of my comments.
</ul> | ||
</header> | ||
|
||
<div class="content"> |
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 <div>
is unnecessary. Give the class and styling to <main>
instead.
Adding <div>
s should always be a last resort when no other element can do the job. In this case, the <main>
element is doing the job!
<div class="content"> | ||
<main> | ||
<h1>Programming Blog</h1> | ||
<!-- <p></p> --> |
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.
Always remove unnecessary comments (especially comments that only serve to comment out code) before submitting a PR.
<!-- <p></p> --> | ||
|
||
<section class="blog-post"> | ||
<h3>The Firsts Month and a Half: A Reflection</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.
Typo: "Firsts" should be "First"
<h1>Programming Blog</h1> | ||
<!-- <p></p> --> | ||
|
||
<section class="blog-post"> |
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.
I would use <article>
here instead of <section>
as it's more semantically relevant to the content.
</ul> | ||
</header> | ||
|
||
<div class="content"> |
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.
Same notes about this <div>
as the one for 1blog.html.
text-align: right; | ||
position: relative; | ||
bottom: 0px; | ||
right: .5%; |
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.
I would use margin-right or padding-right here. Right on its own means to move the entire element to the right, which means that technically .5% of footer is off the page to the left. This could have unintended consequences later. Try adding a border to this h4 inside the footer to see what I mean.
top: -25px; | ||
right: -69.5%; | ||
} | ||
|
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 creative way of forcing the ul into the location you want. I can tell that you tried to make the h2 narrower so that the ul would fit next to it, but when that wouldn't work you brute forced the ul into the position you wanted it by specifying negative top and right values. Try making the h2 display: inline-block and removing the top/right values and just see what happens.
I would also suggest reading about flex-box as a more reliable (except in Internet Explorer) alternative. Here's the best tutorial https://css-tricks.com/snippets/css/a-guide-to-flexbox/
|
||
body main h1 { | ||
margin: auto; | ||
padding: 1.5% 1.5%; |
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.
Here's a helpful explanation on padding (and margin) one-liners from MDN
/* Apply to all four sides */
padding: 1em;
/* vertical | horizontal */
padding: 5% 10%;
/* top | horizontal | bottom */
padding: 1em 2em 2em;
/* top | right | bottom | left */
padding: 2px 1em 0 1em;
Because you're specifying the same value for all sides, you can actually just specify it once and it will apply it everywhere, i.e. padding: 1.5%
|
||
body main section { | ||
margin: 3%; | ||
width: 30%; |
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 margin and widths are getting over ridden by more specific classes below (.about, .content .project). Because the only place that actually uses these margins and widths are the blog posts on the blog post overview page, I would give those posts a class and use the class in the stylesheet. It'll be clearer for others reading your code.
|
||
.content .project { | ||
margin: auto; | ||
width: 100vw; |
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.
Same goes with this one. If you changed line 64 so that it only referenced the blog posts on the blog post overview page, you wouldn't have to reset the width here.
Static Site
Congratulations! You're submitting your assignment!
Comprehension Questions
<article>
,<header>
or<nav>
serve two purposes: they make your HTML easier for other human programmers to read, and they provide structure needed for assistive technologies like screen readers.