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 - Hyunji Kim - Static-Site #27

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

Conversation

ricecakemonster
Copy link

@ricecakemonster ricecakemonster 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? I missed some closing tags(/..)
Why is it important to consider and use semantic HTML? For blind people(users of web reading program) to understand the contents of the website
How did you decide to structure your CSS? Common ones(shared through different files) are all together on the top. Divided css codes by where it belongs to.
What was the most challenging piece of this assignment? Trying to figure out how things align and figure out why a scrollbar was not working with iframe.
Describe one area that you gained more clarity on when completing this assignment Where and when to use div.

@PilgrimMemoirs
Copy link

PilgrimMemoirs commented Mar 30, 2017

Static Site

What We're Looking For

Feature Feedback
Baseline
Appropriate Git Usage Well Done
Answered comprehension questions Well Done
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. Mostly Good - Blog posts should each have their own article tag instead of being list elements.
All images include alternate text Well Done
CSS
Using class and ID names in style declarations Well Done - great naming of classes and ids, it was very clear what the style was doing or what elements it was styling.
Style declarations are DRY Mostly Good - not sure if .target is being used? I didn't see an element with the class name of target anywhere in your HTML. Some styles are being repeated, like font-styles - should be defined once so it's easier to change if you want a different font.
Overall
Animation Love the image bounce! Nice feature and adorable photo :)

Copy link

@KellyPT KellyPT left a comment

Choose a reason for hiding this comment

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

You have really strong git commits!
Lovely photos ❤️
I am truly amazed by your CSS work. I had to look up many of your syntax and selectors. (Maybe I should go back to ADA for a review?).

</nav>
</span>
</header>

Copy link

Choose a reason for hiding this comment

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

Awesome usage of <div> and <span> here in the <header> section. You know the difference well!

<section id="hero">
<img src="../images/mommy.jpg" class = "bounce-in-top" alt="hero image">
<h1 class = "dropshadow">It's Me!</h1>
<p class="center-p">
Copy link

Choose a reason for hiding this comment

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

I'm a bit confused about this start.html and index.html. Which one is the homepage?

<link href ="../styles/styles.css" rel="stylesheet">
</head>
<body id="about">
<main>
Copy link

Choose a reason for hiding this comment

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

Really minor comment: A navigation bar at the top of each page would help me go back and forth between your website pages more easily. :)

<link href ="../styles/styles.css" rel="stylesheet">
</head>
<body>
<main>
Copy link

@KellyPT KellyPT Mar 31, 2017

Choose a reason for hiding this comment

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

How about a navigation bar here?

<link href ="../styles/normalize.css" rel="stylesheet">
<link href ="../styles/styles.css" rel="stylesheet">
</head>
<body>
Copy link

@KellyPT KellyPT Mar 31, 2017

Choose a reason for hiding this comment

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

How about a navigation bar here?

line-height: 1.15; /* 2 */
-ms-text-size-adjust: 100%; /* 3 */
-webkit-text-size-adjust: 100%; /* 3 */
}
Copy link

Choose a reason for hiding this comment

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

Wow 👍

line-height: 0;
position: relative;
vertical-align: baseline;
}
Copy link

Choose a reason for hiding this comment

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

Wow 👍

[type="reset"],
[type="submit"] {
-webkit-appearance: button; /* 2 */
}
Copy link

Choose a reason for hiding this comment

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

Wow 👍

[type="submit"]::-moz-focus-inner {
border-style: none;
padding: 0;
}
Copy link

Choose a reason for hiding this comment

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

👍

[type="reset"]:-moz-focusring,
[type="submit"]:-moz-focusring {
outline: 1px dotted ButtonText;
}
Copy link

Choose a reason for hiding this comment

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

👍

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.

3 participants