London | 26-ITP-May | Vitalii Kmit | Project TV Show | Level 500 - #83
Vitalii-code wants to merge 41 commits into
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
Vitalii-code
added
the
Needs Review
Vitalii-code
changed the title
LonMcGregor
added
the
Review in progress
LonMcGregor
left a comment
LonMcGregor
left a comment
There was a problem hiding this comment.
Good attempt at this task, the implementation works but seems to have some issues.
Did you remember to validate / run the lighthouse checks before submitting?
As I test your implementation, I see that you are realoading the page when you go from state to state. This results in you not being able to make best use of your caching. Can you find an alternative way to change state without reloading the page?
ITP is finished now but if you want to push yourself, you can try to address my comments
|
|
||
| <body> | ||
| <div id="root"> | ||
| <div id="root"></div> |
There was a problem hiding this comment.
There may be better semantic elements you could have used here than a div
There was a problem hiding this comment.
I don't think it's used anywhere, so I'll just remove it.
| <template id="show-card"> | ||
| <article> | ||
| <grid> | ||
| <h1 class="title"></h1> |
There was a problem hiding this comment.
Have a look at what https://axesslab.com/alt-texts/ has to say about alt-texts. If you don't think one is necessary, there's a specific thing you need to do.
| } | ||
|
|
||
| img { | ||
| display: block; |
There was a problem hiding this comment.
The images seem to not line up, if the width of the text for the show differs in length. Can you line them up?
| p { | ||
| margin-left: 15px; | ||
| margin-right: 15px; | ||
| color: #a2a2a2; |
There was a problem hiding this comment.
Is this an appropriate colour to use here?
| episode.image && episode.image.medium | ||
| ? episode.image.medium | ||
| : PLACEHOLDER_IMAGE; | ||
| clone.querySelector(".description").innerText = episode.summary; |
There was a problem hiding this comment.
Is there any difference here between how episodes and shows handle this data from the API? What would the fix be? Which one is the safer approach to use?
| if (episodeList == undefined || episodeList.length === 0) return; | ||
|
|
||
| for (const episode of episodeList) { | ||
| const code = |
There was a problem hiding this comment.
Is this formatting code reused anywhere? Could this be reorganised keeping that in mind?
LonMcGregor
added
Reviewed
This branch has not been deployed
3 participants
Learners, PR Template
https://cyf-vitalii-code-tv.netlify.app/
Self checklist
Changelist
d4ef927 Add a link to go back to home
e85e351 Filter by genre
44b9042 Add button to display more info
869a90a Add information panel for each tv show
2a0a20c Add search to shows
f3a6186 Jump to the show on click
329f175 Style show display
59ef1f7 Display card differently depending on context
89cd9f9 Add placeholder image
ffc066d Hide some controls by default
f43bc3b Add it back because it was in fact necessary
d347f74 Remove unnecessary render() calls
f08915d Parse URL parameters