Manchester | ITP-May-26 | Joanne O'Malley | Sprint 3 | Project TV Show - #85
Manchester | ITP-May-26 | Joanne O'Malley | Sprint 3 | Project TV Show#85backup342 wants to merge 3 commits into
Conversation
joanne342
added
the
Needs Review
LonMcGregor
left a comment
LonMcGregor
left a comment
There was a problem hiding this comment.
Unfortunately you can't make PRs from templates, only forks. This way of submitting is fine for this case, don't worry.
- Good use of caching
- The deployed page has all the right functionality
- Have you tried using lighthouse to see if it has any suggestions about your HTML semantics and accessibility?
Good work on this. ITP is complete now, but if you want an extra challenge, feel free to respond to my comment
| <title>TV Show Project | Joanne O'Malley (joanne342)</title> | ||
| <link href="style.css" rel="stylesheet" /> | ||
| <link rel="icon" href="/favicon.ico" type="image/x-icon" /> | ||
| </head> |
There was a problem hiding this comment.
Most of your HTML is created within the Js. What benefits or tradeoffs do you think there are with this approach?
|
|
||
| card.innerHTML = ` | ||
| <h2>${episode.name} - ${formatEpisodeCode(episode)}</h2> | ||
| <img src="${episode.image?.medium || ""}" alt="${episode.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 = ` | ||
| <h2>${episode.name} - ${formatEpisodeCode(episode)}</h2> | ||
| <img src="${episode.image?.medium || ""}" alt="${episode.name}"> | ||
| <div class="episode-summary">${episode.summary || ""}</div> |
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 controls; | ||
| } | ||
|
|
||
| async function setup() { |
There was a problem hiding this comment.
Unless there's a clear reason for it, having a lot of your functions defined inside setup might not be such a good idea as it leads to a very long difficult to read function. Do you think this approach is the best for your code?
| }); | ||
|
|
||
| // Dual-purpose search (Shows or Episodes based on view) | ||
| searchInput.addEventListener("input", (event) => { |
There was a problem hiding this comment.
Good idea to combine this control
LonMcGregor
added
Reviewed
3 participants
Learners, PR Template
Self checklist
Changelist
This is a mirror of https://github.com/joanne342/Project-TV-Show because that one won't let me do a PR because somehow it seems to be a template of https://github.com/CodeYourFuture/Project-TV-Show rather than a fork. CYF isn't on any of the PR dropdowns. When I try to fork the CYF one to my GitHub (even calling it something different), it says the fork already exists but it's really not anywhere:
https://github.com/CodeYourFuture/Project-TV-Show/forks
I've been going round in circles with Chat GPT for hours and I don't know what I can do to fix this short of deleting everything and starting again which would involve repeatedly swapping with Abdu again so I hope this is acceptable.
Questions
Ask any questions you have for your reviewer.