Skip to content

Feedback on homework #2 #1

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.

Sign up for GitHub

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

Open
thatdevgirl wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Feedback on homework #2 #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions starter_code/css/style.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,14 @@
/*
Hi Valentine! I added feedback about your code in CSS comments like this one.

Overall, fantastic job with this! Your CSS code is well organized (also thanks to the way you
named your classes in the HTML files) and well indented.

More comments below! (Although, I really do not have too many comments for your CSS. It largely
looks good and functions as it should.)
*/


/*
Project Name: Blank Template
Client: Your Client
Expand Down Expand Up @@ -49,10 +60,20 @@ body {
color: #000;
font-size: 12px;
line-height: 1.4;

/*
I saw that you included the link to the Open Sans Google font in your HTML, but you also
need to tell your CSS that you want to use that font here, in the font-family rule.
*/
font-family: Helvetica, Arial, sans-serif;

background-size: cover;
text-align: center;
/*
I did see that there is a ton of white space around the entire page. This padding rule is why.
Since you are adding 200px of padding inside of each side of the <body> tag, that means that
the elements inside of the body tag start and end 200px from the edge of the page.
*/
padding: 200px;

}
Expand All @@ -73,6 +94,25 @@ body {
header {
text-align: center;
background: url(../images/header_bg.jpg) no-repeat center top rgb(134,141,157);
/*
This background-position rule is actually overwriting the positioning you set up in the background
rule in the line above.

The background rule above is setting the position of the background to
"center top", which means the background is horizontally centered and vertically aligned to the top.

The background-position rule below is saying that the background is both horizontally and vertically
centered.

That being said, putting everything in the background rule (above) can be a little confusing because you can
have a lot of information in just that one line, which can make it harder to read. It might be easier to
separate out all of the elements like this instead:

background-image: url('../images/header_bg.jpg');
background-repeat: no-repeat;
background-position: center top;
background-color: rgb(134,141,157);
*/
background-position: center;
padding:50px;

Expand Down Expand Up @@ -138,6 +178,10 @@ h3{
.testimonial{
text-align: center;
background-image: url(../images/testimonial_bg.jpg);
/*
Probably a typo, but make sure all of your rules always end with a semi-colon, even if it is the
last rule in the block.
*/
padding: 40px
}

Expand Down
36 changes: 36 additions & 0 deletions starter_code/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,17 @@
<!--
Hi Valentine! I added feedback about your code in HTML comments like this one.

Overall, great job! The page looks just like the design file and your code is very neat,
organized, and well indented, which makes it very easy for me to follow.

Another overall comment - I noticed you did not use the <nav> tag in here. I completely understand
why - there is no obvious navigation (though you could make the case that the social media icons are
a "navigation" to the links to those site). I just wanted to make sure you are aware of the <nav> tag
for future projects that have a more obvious navigation.

More comments below!
-->

<!DOCTYPE html>
<html lang="en">
<head>
Expand All @@ -17,12 +31,30 @@
<body>

<div id="container">
<!-- Great use of the header tag here -->
<header>
<h1 class="whiteTextLogo">Relaxr</h1>
<!--
Technically, the tag line below is not a sub-topic of the main topic of this page ('Relaxr'),
so it should not be using the <h2> tag. It is best to use a <p> tag instead.
-->
<h2>Get piece of mind with a single tap</h2>
<!--
Since this "Get it now" button would probably go to another page, this should be
an <a> tag instead. You can always style <a> tags to look like <button> tags, though
I completely get why you chose the <button> tag here. This wasn't particularly clear
in the design file, so don't worry about it too much in this case, but something to
think about for future projects.
-->
<button>Get it now</button>
</header>

<!--
<div> tags are totally fine here, but it is more semantically correct to use something
like the <section> tag instead, since these blocks of stuff on the page are more like
sections of the page. The <div> tag is usually used when no other HTML5 tags make sense -
kind of like the "other" category in surveys.
-->
<div class="greyBackground">
<h3>Benefits</h3>

Expand All @@ -37,6 +69,10 @@ <h3>Benefits</h3>
</div>


<!--
This applies to all of the classes in these sections, but I really like how you organized
the names of your classes for each section so that they relate to each other.
-->
<div class="testimonial">
<p class="testimonialQuote">“Relaxr changed my life. I’ve been able to travel the world, spend limited time working and my boss keeps thanking me for crushing work.”</p>
<p class="testimonialName"> - Amanda, Intuit</p>
Expand Down