-
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
Ashton's Static Site #37
base: master
Are you sure you want to change the base?
Conversation
Static SiteWhat We're Looking For
|
<body> | ||
|
||
<header> | ||
<img id="logo" src="../Assets/static-logo.png" alt="Ashtn"> |
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.
The "title" attribute is what shows up when you hover over something - that should say "Ashtn." The "alt" attribute should be as simple as possible for a screen reader - it won't necessarily know how to pronounce "Ashtn" without the "o", so I recommend changing this to "Ashton."
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.
Regardless, good use of alt tags - you'd be amazed how many folks forget or neglect to include accessibility in their builds.
<meta charset="utf-8"> | ||
<title>Ashtn</title> | ||
<link rel="stylesheet" href="../Styles/normalize.css"> | ||
<link rel="stylesheet" href="../Styles/styles.css"> |
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 advice will come in handy later when you're using a preloaded framework like Foundation or Bootstrap, but there's often a "reset.css" file that's necessary to include in order to remove weird shit the browser likes to pull and give you a truly blank slate re: styling. Foundation 6 (iirc) does it for you automatically, but it's worth checking on depending on the version and framework you're specifically using.
Call reset.css first and then move on down the line. I'm not sure if normalize is the equivalent of reset here or what. This is why I'm a back-end dev XD
@@ -0,0 +1,68 @@ | |||
<!doctype html> |
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.
Side note - when it comes to social media, go for .svg files. They're scalable and you can crack open their core code and change the color to whatever you want :D
|
||
</main> | ||
|
||
<footer> |
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.
Okay re: social media links. First quick thing - there's a way to note if your links to things like google etc open in another browser tab or if the low-vision/blind person should expect that clicking it means they leave this page in the same window. It's confusing as hell when it's the unmarked latter because they're like "wat" when the thing they thought they were looking at has unexpectedly changed.
Second - a recommendation for your social media is that you pick one layout for it and then copy/paste it in the footer everywhere you want it. There's inconsistencies across your pages right now - in your about page they're free-floating in a section outside the footer, they're in your footer with an id in your index page, and your projects page has no id on its footer.
Third - just a few lines up on 81 (under section class="project-foot") you misspelled a few things: "Link to Github repository."
|
||
header { | ||
position: fixed; | ||
background: #fff; |
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.
Clever! I didn't learn about abbreviating hex colors that had the same letter until much later.
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.
Also, you should very rarely need to repeat yourself with regards to color in the same section (site header, for example). You don't always have to include the body for the header - you could just target the header itself to control its color. That way your code stays DRY and it's easier to read and make changes.
|
||
#logo { | ||
float: left; | ||
width: 100px; |
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.
Why did you use % width up above for header and pixel width for logo?
|
||
header ul { | ||
float: right; | ||
padding: 4em 2em 0 0; |
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 recommend using % for padding and borders and rem for font sizes - this is what Jamie strongly suggested.
margin: 0; | ||
padding: 0; | ||
display: inline; | ||
height: 40px; |
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'd recommend standardizing the sizing format (%, px, em/rem) you use to set up heights and widths. One thing I used to do when making my .css file was to take all of the classes and ids in a section and cross off duplicate lines - it helped me to see when things weren't as DRY as they could be. Having standardized sizing formats helped make it obvious when I was repeating myself unnecessarily.
You're missing H1 tags, which may cause screen readers to have a hard time orienting themselves on your site. Your .css file could use some trimming and DRYing up, but overall solid project. 👍
Static Site
Congratulations! You're submitting your assignment!
Comprehension Questions