derpachu / MI-449-SS17-740-html-more-kinds-of-content-OSB7Pe

0 stars 0 forks source link

Project Feedback #1

Open derpachu opened 7 years ago

derpachu commented 7 years ago

@chrisvfritz Can you take a look at this? It's hosted here and meets the following criteria:

let me know if anything is not up to standards

chrisvfritz commented 7 years ago

Looks beautiful! 🌟 As a fan of both the Sim City games and Cities: Skyline, I look forward to this next evolution in the genre. 😄 I only have a couple notes regarding the table:

<table>
<tr>
  <th></th>
  <th>number of cities</th>
  <th>number of catastrophes</th>
</tr>
<tr>
  <td>this game</td>
  <td>8</td>
  <td>32</td>
</tr>
<tr>
  <td>Simcity</td>
  <td>1</td>
  <td>8</td>
</tr>
<tr>
  <td>cites: skyline&nbsp;</td>
  <td>4</td>
  <td>0</td>
</tr>
</table>

Indentation inside table

The elements inside the table element are not indented further, to indicate they're children.

Tables headings and thead

This row only includes headings for the entire table:

<tr>
  <th></th>
  <th>number of cities</th>
  <th>number of catastrophes</th>
</tr>

So semantically, it would probably be best to wrap it with a thead element, then put the rest of the rows in a tbody element.

Missing th element(s)

The first column (containing this game, Simcity, and cities: skyline) doesn't have a heading of its own, indicating that this is probably a column of headings. For this reason, I would either:

  1. Update these cells to be th elements instead of td elements, OR
  2. Put a heading such as Name in the top row

Let me know if you have any questions or when the code is ready for another look. 🙂

derpachu commented 7 years ago

reupdated the table to fix issues

chrisvfritz commented 7 years ago

Very nice! 🎉 👍 🎈 :shipit: