Ericka & Janice Group Fansite- Sapphire #33
Conversation
Co-authored-by: janice15 <[email protected]>
There was a problem hiding this comment.
Good work on this project, y'all!
I appreciate the practice of CSS grid and HTML overall.
A lot of my feedback is around having consistently quality HTML; always follow best practices around semantic HTML and getting the details right. A lot of my comments are about specific items. Please let me know what questions and comments come up.
Good work with the coworking agreement and your git hygiene as well.
Wooooooo!
| <div class="item1"> | ||
| <img src="/images/meredith-grey.jpg" alt="Meredith Grey"> | ||
| <p class="character">Meredith Grey</p> | ||
| </div> | ||
| <div class="item2"> | ||
| <img src="/images/cristina_yang.jpg" alt="Cristina Yang"> | ||
| <p class="character">Christina Yang</p> | ||
| </div> | ||
| <div class="item3"> | ||
| <img src="/images/alex-karav.jpg" alt="Alex Karev"> | ||
| <p class="character">Alex Karev</p> | ||
| </div> | ||
| <div class="item4"> | ||
| <img src="/images/george-omalley.jpg" alt="George O'malley"> | ||
| <p class="character">George O'malley</p> | ||
| </div> | ||
| <div class="item5"> | ||
| <img src="/images/izzie-stevens.jpg" alt="Izzy Stevens"> | ||
| <p class="character">Izzie Stevens</p> | ||
| </div> | ||
| <div class="item6"> | ||
| <img src="/images/derek-shepard1.jpg" alt="Derek Shepard"> | ||
| <p class="character">Derek Shepard</p> | ||
| </div> | ||
| <div class="item7"> | ||
| <img src="/images/miranda-bailey.jpg" alt="Miranda Bailey"> | ||
| <p class="character">Miranda Bailey</p> | ||
| </div> | ||
| <div class="item8"><img src="/images/richard-webber.jpg" alt="Richard Webber"> | ||
| <p class="character">Richard Webber</p> | ||
| </div> |
There was a problem hiding this comment.
Each of these different divs has unique classes, item1, item2, ... item8.
- If they are unique, I would expect these to be IDs, not classes
- The classes
item5and up aren't used anywhere, so I would expect these to be deleted
| body { | ||
| background-color: lightblue; | ||
| } | ||
|
|
||
| p { | ||
| font-family: Arial, Helvetica, sans-serif; | ||
| font-size: 20px; | ||
| color: black | ||
| } | ||
|
|
||
| .nav { | ||
| color: darkslategrey; | ||
| text-align: left; | ||
| background-color: lightblue; | ||
| display: inline-block; | ||
| border-style: dotted; | ||
| border-color:lightseagreen; | ||
| } | ||
|
|
||
| img { | ||
| width:150px; | ||
| height: auto; | ||
| } | ||
|
|
There was a problem hiding this comment.
Throughout the project, there seems to be inconsistent tab-spacing. It'd probably be good to conform to 2 or 4 spaces for tabs.
| <link rel="stylesheet" href="/styles/styles.css"> | ||
| <link rel="stylesheet" href="/styles/index.css"> | ||
| <title>Document</title> |
| <a href=/pages/gallery.html rel="import">Gallery</a> |
There was a problem hiding this comment.
What does rel="import" do? Should this be here?
| <h1>Janice and Ericka's Grey's Anatomy Fan Page!</h1> | ||
| <h3>Gallery</h3> |
There was a problem hiding this comment.
For semantic HTML, I'd expect there to be h2 used before h3. If you need it to be h3 in order to have a better font size, you should use h2 still and use CSS to change the text size.
This goes throughout your project
| <div class="item1"> | ||
| <img src="/images/meredith-grey.jpg" alt="Meredith Grey"> | ||
| <p class="character">Meredith Grey</p> | ||
| </div> | ||
| <div class="item2"> | ||
| <img src="/images/cristina_yang.jpg" alt="Cristina Yang"> | ||
| <p class="character">Christina Yang</p> | ||
| </div> |
There was a problem hiding this comment.
Considering how all these items have identical structure, I would really recommend using that to your advantage.
What if the div had a reused class, like character-item instead of a unique item1 class?
Then, what if instead of class="character" on each paragraph, you could select the paragraph child of any element with the character-item class?
No description provided.