London | May-26 | Liridona Shehu | Sprint 3 | Module-Data-Flows | Tv-Show -Project | Level_500 - #88
London | May-26 | Liridona Shehu | Sprint 3 | Module-Data-Flows | Tv-Show -Project | Level_500#88shehu-dona wants to merge 39 commits into
Conversation
Level0/100. Merging the changes to main for level0/100
London | 26-ITP-May | Martin Mwaka | Sprint 1| Project-TV-Show Level 200
London | 26-ITP-May | Martin Mwaka | Sprint 1 | Level 200 - add search input
London | 26-ITP-May | Martin Mwaka | Sprint 2| Input and Select
London | May-26 | Liridona Shehu | Sprint 2 | Module-Data-Flow | Tv-Project| Level 300
… 400 (#6) * udpate index, script and css files * update index.html
London | May-26 | Liridona Shehu | Sprint 3 | Module-Data-Flows | Tv-Project | Level_500_Dona
fixed show select dropdown
cjyuan
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.
- Appropriate use of semantic HTML
- The deployed sit has all the necessary functionality
- Generally the js code is good, but I have some comments
Good work on this. ITP is complete now, but if you want an extra challenge, feel free to respond to my comments.
| async function fetchShows() { | ||
| if (showsCache) return showsCache; | ||
|
|
||
| if (!showsPromise) { |
There was a problem hiding this comment.
Why do you use this if block checking if a promise is present?
| const card = document.createElement("section"); | ||
| card.className = "show-card"; | ||
| card.innerHTML = ` | ||
| <h3>${show.name}</h3> |
There was a problem hiding this comment.
Is there a reason you are using HTML in the js here, given you also use a template?
| card.className = "show-card"; | ||
| card.innerHTML = ` | ||
| <h3>${show.name}</h3> | ||
| <img src="${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
| card.innerHTML = ` | ||
| <h3>${show.name}</h3> | ||
| <img src="${show.image?.medium || ""}" alt=" ${show.name}"> | ||
| <p>${show.summary || ""}</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?
| return episodesCache.get(showId); | ||
| } | ||
|
|
||
| if (!episodesPromises.has(showId)) { |
There was a problem hiding this comment.
What benefit is there in having 2 different caches here?
| const episodeImage = episodeCard.querySelector("img"); | ||
| const episodeSummary = episodeCard.querySelector("p"); | ||
|
|
||
| title.innerText = `${episode.name} - ${formatEpisodeCode( |
There was a problem hiding this comment.
The code here is formatted a bit oddly. If you want to split it over multiple lines, is there a way to do so that retains the readability better?
| showEpisodesView(Number(selectedValue)); | ||
| }); | ||
|
|
||
| document.querySelector(".back-to-shows").addEventListener("click", () => { |
There was a problem hiding this comment.
Can you think of any alternative approaches that would simplify changing the state of classes, so you don't need to individually change each one?
LonMcGregor
added
Reviewed
5 participants
Self checklist
Changelist
A PR for Tv_Project_Level_500.