-
Notifications
You must be signed in to change notification settings - Fork 155
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
Cheetahs-Wanjun Lan #97
base: master
Are you sure you want to change the base?
Conversation
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.
Personal Portfolio Site
What We're Looking For
Feature | Feedback |
---|---|
Baseline | |
Appropriate Git Usage | ✅ |
Answered comprehension questions | ✅ |
Page fully loads | ✅ |
No broken links (regular or images) | Not quite, see comments |
Includes at least 3 pages and styling | ✅ |
HTML | |
Uses the high-level tags for organization: header, footer, main |
✅ |
Appropriately using semantic tags: section, article , etc. |
Not quite, see comments |
All images include alternate text | Almost |
CSS | |
Using class and ID names in style declarations | ✅ |
Style declarations are DRY | Mostly |
Overall | ✅ |
color:rgb(221, 151, 151); | ||
} | ||
|
||
a { |
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.
There is already an a
CSS style rule on line 63. These rules can be added there to make things DRY.
<li><a href="http://127.0.0.1:5502/pages/index.html"><h3>Home</h3></a></li> | ||
<li><a href="http://127.0.0.1:5502/pages/about.html"><h3>About</h3></a></li> |
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 links do not work. We should use relative paths to reference other pages, similar to what you have on line 16 to get to the assets. Here is a great resource on absolute vs relative paths: https://www.geeksforgeeks.org/absolute-relative-pathnames-unix/
</div> | ||
<div class="about-main-container"> | ||
<div> | ||
<img class="me-photo" alt="fuji" src="../assets/Fuji.JPG"> |
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.
It would be good for alt text to be more descriptive. Alt text is used by screen readers for people who are blind to explain to them what the picture is. Just saying "Fuji" is not enough to help someone who is blind understand what is in the picture.
<div> | ||
<div class="index-main-container"> | ||
<div> | ||
<a href="/about.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.
This link also does not work because it is an absolute path instead of a relative path. Please see the comment above about absolute vs relative.
</ul> | ||
</nav> | ||
</header> | ||
<div id="div-grid"> |
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.
It seems there are a lot of divs being used on your pages. Instead, please consider using semantic tags like <section>
or <article>
. Screen readers tell blind users when a new section or a new article starts. It does not do that with <div>
. Users will not know that they are on a new section of your page.
Personal Portfolio Site
Congratulations! You're submitting your assignment!
Comprehension Questions