London | 26-ITP-May | Jorvan White | TV SHOW PROJECT | LEVEL 500 - #77
London | 26-ITP-May | Jorvan White | TV SHOW PROJECT | LEVEL 500#77JorvanW wants to merge 18 commits into
Conversation
Complete Level 0 setup
Level 200 khaliun
LONDON | ITP-May-26 | Jorvan White | Sprint 4 | Project TV Show | Level 300
feat: add multiple TV show support
JorvanW
added
the
Needs Review
LonMcGregor
left a comment
LonMcGregor
left a comment
There was a problem hiding this comment.
- The JS code is well organised
- The deployed site has all the functionality required
- Good handling of state transitions
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"> |
There was a problem hiding this comment.
Did you check if the lighthouse tool offers any advice for creating a select this way?
| <div id="shows-view"> | ||
| <h1>TV Shows</h1> | ||
|
|
||
| <input |
There was a problem hiding this comment.
Is there HTML that is more semantic in design that you could use?
| showBox.className = "show"; | ||
|
|
||
| showBox.innerHTML = ` | ||
| <img |
There was a problem hiding this comment.
What do you think are the benefits are of definine HTML within JS like this?
| showBox.innerHTML = ` | ||
| <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
| </a> | ||
| </h2> | ||
|
|
||
| <p>${show.summary || "No summary available."}</p> |
There was a problem hiding this comment.
This isn't wrong, but can you think of any potential side effects of inserting the summary from the API directly into your HTML like this?
LonMcGregor
added
Reviewed
3 participants
Self checklist
Changelist
My level 500 PR following the requirement of the module