Lion - Miranda #115
Open
Lion - Miranda #115
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
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) | ✅ |
| Includes at least 3 pages and styling | ✅ |
| HTML | |
Uses the high-level tags for organization: header, footer, main |
😞 Only footers, no header or main |
Appropriately using semantic tags: section, article, etc. |
😞 Use section and article instead of div |
| All images include alternate text | ✅ |
| CSS | |
| Using class and ID names in style declarations | 😞 Only uses class, no IDs |
| Style declarations are DRY | Mostly |
| Uses Flexbox and/or Grid | ✅ |
| Overall | 👍 |
Comment on lines
+12
to
+16
| <ul> | ||
| <a href="index.html" target="_top"> Home </a> | ||
| <a href="about.html" target="_top"> About </a> | ||
| <a href="portfolio.html" target="_top"> PROTFOLIO </a> | ||
| </ul> |
There was a problem hiding this comment.
When you have an unordered list (ul element), there must be list item (li) elements underneath before you can add things like links. So it should look like this:
<ul>
<li><a href=......></li>
<li><a href=......></li>
</ul>
|
|
||
| <div class="project"> | ||
| <div class="project_pic"> | ||
| <a href="https://github.com/yxzhang88/personal-portfolio-site" target="_blank"> <img src="../assets/view.png" alt="viewing party" height="300px"> <button>Viewing Party</button> </a> |
There was a problem hiding this comment.
- The height attribute of the
imgelement assumes it is pixels, so you should not addpxat the end of the number. It causes an error in the HTML validator. - The
buttonelement cannot be inside anaelement. Theatag already makes it click-able, you do not need to say it is a button. This causes errors. - The alt text for images should be more descriptive. People who are blind or have low vision depend on screen readers to tell them what is on the page. Screen readers use the alt text to explain to the user what the picture is about. Just saying "viewing party" does not explain to the user what the picture is about.
| <div class="inner_container"> | ||
| <div class="past"> | ||
| <h1> The Past</h1> | ||
| <img class="surrounding1" src="../assets/past.jpeg" alt="miranda-selfie" width="35%"> |
There was a problem hiding this comment.
The width attribute of the img tag is in pixels. You cannot use the % symbol, it causes errors.
| As a new immigrants, in these 3 years, 'language' learning is priority in my life constantly. Not only study the English to communicate with people, also study the programming language and try to communicate with computers. | ||
| </p> | ||
| <p> | ||
| I'm a person who likes to continuously trying new things. As Jack in the Titanic said, <em><u>"I figure life's a gift and I don't intend on wasting it. You don't know what hand you're gonna get dealt next. You learn to take life as it comes at you. To make each day count".</em></u> |
There was a problem hiding this comment.
Generally, tags should open and close in order if they are right next to each other. Since em was opened first, it should close first. The ordering is causing errors.
| padding: 0 10px 0 0; | ||
| } | ||
|
|
||
| .surrounding3 { |
There was a problem hiding this comment.
Since these properties are the same as .surrounding1, you do not need this one. Everything with .surrounding3 as the class can just use 1.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Personal Portfolio Site
Congratulations! You're submitting your assignment!
Comprehension Questions