Conversation
London | 26-ITP-May | Zadri Abdule | TV Show Project | Level-200
London | 26-ITP-May | Zadri Abdule | Project TV Show | Level 400
London | 26-ITP-May | Zadri Abdule | Project TV Show | Level 400
London | 26-ITP-May | Vitalii Kmit | Project TV Show | Level 500
Zadri415
added
the
Needs Review
LonMcGregor
added
the
Review in progress
LonMcGregor
left a comment
LonMcGregor
left a comment
There was a problem hiding this comment.
- Good use of caching
- Functionality works well
I have spotted some issues in the code. ITP is over now, but if you want some extra practice, feel free to respond to the comments.
|
|
||
| <body> | ||
| <div id="root"> | ||
| <div id="root"></div> |
There was a problem hiding this comment.
Did you try running the lighthouse check? Does it have any suggestions about better semantic html you could use here?
| <script src="episodes.js"></script> | ||
| <template id="show-card"> | ||
| <article> | ||
| <grid> |
There was a problem hiding this comment.
Is "grid" an appropriate HTML element to use here?
| for (const episode of episodeList) { | ||
| const code = | ||
| "S" + | ||
| String(episode.season).padStart(2, "0") + |
There was a problem hiding this comment.
This formatting code is repeated in a couple of places, could you find a way to abstract this and make it reusable?
| } | ||
| } else { | ||
| // display show search | ||
| await fetch(SHOWS_URL) |
There was a problem hiding this comment.
You have two fetches for the shows_url here - is there a reason you need to include this twice?
| setupShowSearch(); | ||
|
|
||
| showSelect.addEventListener("change", () => { | ||
| window.location.href = `/?showId=${showSelect.value}`; |
There was a problem hiding this comment.
Does this do anything? I don't see any conditions where the query on your page seems to change. When should this code activate?
| <template id="episode-card"> | ||
| <article> | ||
| <h1 class="title"></h1> | ||
| <img class="thumb" /> |
There was a problem hiding this comment.
When I'm looking at your images, do they have appropriate alt tags? Does https://axesslab.com/alt-texts/ have any useful guidance here?
LonMcGregor
added
Reviewed
This branch has not been deployed
3 participants
Learners, PR Template
Self checklist
Changelist
Implemented the level 500 shows-listing feature:
Added a shows listing view on app start, showing name, image, summary, genres, status, and rating/runtime for all shows
Added free-text search across show name, genres, and summary
Clicking a show name fetches and displays its episodes (search + select still work as before), hiding the shows listing
Added a "Back to shows" control to return to the listing, hiding the episodes view
Episodes are cached per show so no show's episodes are fetched more than once per visit
Fixed a broken tag for episode cards that would have crashed the episodes view
Accessibility fixes: added alt text to all images, made the clickable show name a real so it's keyboard-focusable
Fixed episode summaries rendering as raw
tags instead of formatted HTML
Questions
N/A