London | 26-ITP-May | Khaliun Baatarkhuu | Sprint 3 | Project TV Show - #79
London | 26-ITP-May | Khaliun Baatarkhuu | Sprint 3 | Project TV Show#79khaliun-dev wants to merge 16 commits into
Conversation
Complete Level 0 setup
LONDON | ITP-May-26 | Jorvan White | Sprint 4 | Project TV Show | Level 200
feat: fetch episodes from TVMaze API
LONDON | ITP-May-26 | Jorvan White | Sprint 4 | Project TV Show | Level 400
London | 26-ITP-May | Khaliun Baatarkhuu | SPRINT 3 | TV Project
khaliun-dev
added
📅 Sprint 3
khaliun-dev
changed the title
fix: restore level 400 show selector
LonMcGregor
left a comment
LonMcGregor
left a comment
There was a problem hiding this comment.
- Good use of caching
- The deployed site has all the right functionality
- The code is generally well laid out
Good work on this. ITP is complete now, but if you want an extra challenge, feel free to respond to my comments.
| placeholder="Search shows" | ||
| /> | ||
|
|
||
| <select id="show-selector"></select> |
There was a problem hiding this comment.
Did you check if the lighthouse tool offers any advice for creating a select this way?
| showsList.innerHTML = ""; | ||
|
|
||
| showList.forEach(function (show) { | ||
| const showBox = document.createElement("div"); |
There was a problem hiding this comment.
You generally made good use of semantic HTML, is there more specific semantic HTML you could use here instead of a div
|
|
||
| showBox.className = "show"; | ||
|
|
||
| showBox.innerHTML = ` |
There was a problem hiding this comment.
What do you think are the benefits are of defining HTML within JS like this?
|
|
||
| <img | ||
| src="${show.image ? show.image.medium : ""}" | ||
| alt="${show.name}" |
There was a problem hiding this comment.
Is this an appropriate use of an alt tag? See what https://axesslab.com/alt-texts/ has to say
There was a problem hiding this comment.
What could be changed to make the style of episodes and shows match each other better?
There was a problem hiding this comment.
Is there any change you could make that makes it clearer that you can click on shows to have something happen?
| }); | ||
| } | ||
|
|
||
| async function setup() { |
There was a problem hiding this comment.
This setup function is quite large. What changes could help break it up while retaining readability?
LonMcGregor
added
Reviewed
2 participants
Changelist
Tested
git diff --checkQuestions
None.