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

Alison Z's static site #33

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

Alison Z's static site #33

wants to merge 2 commits into from

Conversation

AlisonZ
Copy link

@AlisonZ AlisonZ 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? Oops! I didn't do this. I'll have to go back and do that.

| Why is it important to consider and use semantic HTML? | So that your site is easy to understand and organize |
| How did you decide to structure your CSS? | I tried to do a main page and then different style pages for each page |
| What was the most challenging piece of this assignment? | Finishing it! I did not. I got too distracted and spent hours doing little things. It was so easy to get sidetracked on this. |
| Describe one area that you gained more clarity on when completing this assignment | FLOATS!! |

@CheezItMan
Copy link

Static Site

What We're Looking For

Feature Feedback
Baseline
Appropriate Git Usage I'm only seeing 2 commits, this is definitely something to work on.
Answered comprehension questions Not validated HTML, It's usually best to have a single CSS for the site, so that the entire site has a similar appearance. With separate css files it's way too easy for each page to have an inconsistent look.
Page fully loads Index loads well, the other pages are just stubs at present.
No broken links (regular or images) No broken links although some link to stub pages. It would be nicer if the footer links were icons or something.
Includes at least 4 pages and styling Really only 1 page of content right now.
HTML
Uses the high-level tags for organization: header, footer, main body should be an element enclosing all the content, header, footer and main, and where you're using body you should be using main.
Appropriately using semantic tags: section, article, etc. You're doing some semantic HTML. It would be a good idea to nest your header links inside articles and sections inside header tags h1, h2, ...
All images include alternate text No alt attributes used. Note the HTML validator will point this out as well.
CSS
Using class and ID names in style declarations No classes used at all
Style declarations are DRY CSS is DRY, good use of the splat (*) to set the universal font family.
Overall A good start, but it doesn't seem you are finished yet.

<body>

</body>
</html>

Choose a reason for hiding this comment

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

This one has the correct tag structure for head and body.

@@ -0,0 +1 @@
<svg xmlns="http://www.w3.org/2000/svg" data-name="Layer 1" viewBox="0 0 100 125" x="0px" y="0px"><title>tri 5</title><path d="M51.94,86.74,14.14,21.27l-5.5-9.53L7.35,14h86.6l-1.3-2.26L54.85,77.21l-5.5,9.53a1.5,1.5,0,0,0,2.59,1.51l37.8-65.47,5.5-9.53A1.51,1.51,0,0,0,93.95,11H7.35A1.51,1.51,0,0,0,6,13.26l37.8,65.47,5.5,9.53A1.5,1.5,0,0,0,51.94,86.74Z"/><path d="M6.53,68.11l76,.31,10.9,0a1.5,1.5,0,0,0,0-3l-76-.31-10.9,0a1.5,1.5,0,0,0,0,3Z"/><text x="0" y="115" fill="#000000" font-size="5px" font-weight="bold" font-family="'Helvetica Neue', Helvetica, Arial-Unicode, Arial, Sans-serif">Created by Ayse Muskara</text><text x="0" y="120" fill="#000000" font-size="5px" font-weight="bold" font-family="'Helvetica Neue', Helvetica, Arial-Unicode, Arial, Sans-serif">from the Noun Project</text></svg>

Choose a reason for hiding this comment

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

It sounded like you wanted to credit the artist for your images. I think it's fine to put this at the bottom of the image.
Crediting the artist is something you should definitely do. And also ask for permission to use the art if permission is not explicitly given. It's the right thing to do, and it wil save you from copyright issues. Or, use art from the public domain.

<body>

</body>
</html>

Choose a reason for hiding this comment

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

Your secondary pages are empty stubs. I like that they are present because they can help you stay organized.

Copy link

@guineveresaenger guineveresaenger left a comment

Choose a reason for hiding this comment

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

Hi Alison! Here's my overall feedback, there is feedback sprinkled in your code as well:

Baseline

Appropriate Git Usage

Commit early, commit often! I have a not-baseless fear of accidentally wiping everything from my repo using some powerful git command, but the good news is that just using git commit will never do that, plus you will have an impressive log of everything you accomplished.

Comprehension questions

Semantic HTML is really just a convention, which means everyone's life will be better if everyone uses the same convention. This is super important for collaboration.

In terms of finishing this project. It is super easy to go deeper and deeper down a rabbithole. I would recommend that for future projects, you set yourself a timeline with daily goals, and stick to them. Staying on track, knowing which features to cut, learning what is a hard requirement and what is not, are all good skills to have. I would say it is definitely preferable having a finished project than having perfect thumbnail images.

Links and page load

I love the image links at the bottom, and that they change on hover. Your secondary pages are not completed.

Accessibility

Alt text on images is missing. Color contrast is good and passes the HTML Validator. About the only thing I might change there is that the nav bar links upon hover change to a color very like the background, which makes them harder, not easier, to notice.

Overall

I would recommend a bit more comment cleanup before submitting your code; there's some leftovers in your CSS.

I really like the design of the index page. The images and colors are gorgeous and work well, and the site has a clean feel and a logical layout.

footer {
background-color: #4ABDAC;
/*clear: both;*/
}

Choose a reason for hiding this comment

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

I only see a single CSS file. I can see why you'd want to add separate files for each page - if you want to go that way, I would have one overall CSS file for all pages, so you need only specify the styles for things like the nav bar and the footer once and then if one page has special layout needs, put that in a separate file(and name it accordingly, e.g. portfolio.css).
I don't see any classes; a good place to use a class would be for styling the header and the footer similarly, or a special font for all <h2> tags or something. There isn't a whole lot of repetition in your CSS, but perhaps set the 100% width in a class called .width-of-page or something.

<section id="image">
<img src="https://unsplash.it/400">
</section>

Choose a reason for hiding this comment

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

Image should have alt text. Also I am really not sure I would put one single image inside a <section> tag. I'd probably go with a <div> for that.

</body>
<footer>
<section id="disclaimer">
<p>Copyright</p>

Choose a reason for hiding this comment

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

The copyright symbol in html: &copy;.

margin-bottom: 0px
}

header nav ul li {

Choose a reason for hiding this comment

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

This is a perfect place to put an id on the <li> tag. Call it nav-links and use that in your CSS. Make it an obvious name, and we won't have to look for each element in turn. It makes sense that navbar list items are special - use an ID to show that.

}

header nav li a:hover {
color: #99D3DF;

Choose a reason for hiding this comment

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

ID would make this easier.

background: hotpink;
}

#more_info article a span {

Choose a reason for hiding this comment

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

This is a similar probem - you aren't really getting a lot of use out of your ID #more_info, how about you ID the span instead?
If you want to logically group the sections, give them a class identifier.

<a href="#">LinkedIn</a>
</section>
</footer>
</html>

Choose a reason for hiding this comment

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

What Chris said - an HTML file has a <head> and a <body> and everything that's not in the header goes in the body. I Googled "semantic HTML outline cheatsheet" under Images, and found this site: http://www.hongkiat.com/blog/html-5-semantics/ - scroll down for a visual example of a website with HTML tags in the right places.
Overall, I feel there are too many section tags, and I think you feel this way too, given that you gave each a unique ID to differentiate. I'm honestly not sure what is best practice here; this is something to reconsider and ask about.


footer {
background-color: #4ABDAC;
/*clear: both;*/

Choose a reason for hiding this comment

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

You got the footer to stick to the bottom and a fixed header! Yay! I see that you had trouble with this.

QUESTIONS:::::
-what is up with the auto margin feature as a centering device?
-i want for the three images on the index page to have the images disappear or appear on hover; also want these boxes to be clickable links, so can't click on a disappeared image
-going crazy not being able to center the titles for these three boxes - anywhere would be cool, above, below, inside, just centered!

Choose a reason for hiding this comment

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

I love that you have a file full of references to colors and design decisions just sitting around. That's such a good idea. For the future, I might suggest adding it to your gitignore, since this is basically your sandbox-of-ideas file, and you might not want that out in public. But I think it is very handy and now I know it's a thing, and I'm glad I saw it.

@guineveresaenger guineveresaenger removed their assignment Jan 20, 2023
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