LONDON | ITP-May-26 | Edina Kurdi | Sprint 3 | Project TV Show - #91
edinakurdi wants to merge 87 commits into
Conversation
level-0: add name and username
Level 100
Search functionality
Select Dropdown Menu Functionality
-set up state, -update setup function (including render())
update search and episode selector event listener
edinakurdi
added
the
Needs Review
LonMcGregor
left a comment
LonMcGregor
left a comment
There was a problem hiding this comment.
- Good state management
- Appropriate use of semantic HTML
- Good use of caching
Good work overall. I have a few suggested areas you could look at if you want a bit more practice now that ITP is finished.
| } | ||
|
|
||
| footer a { | ||
| color: #a7f3d0; |
There was a problem hiding this comment.
Be careful here, the lighthouse accessibility check is giving a warning because of this
| episodesView.hidden = false; | ||
|
|
||
| // Reset search and episode dropdown state | ||
| state.episodeSearchTerm = ""; |
There was a problem hiding this comment.
Here you are resetting the search terms, is the intent to always reset this when the state changes? Does this actually update the search box when changing state back and forth?
| // ----------------------------------------------------- | ||
| function populateShowSelect(shows) { | ||
| const optionsHtml = shows | ||
| .map((show) => `<option value="${show.id}">${show.name}</option>`) |
There was a problem hiding this comment.
One important thing to note if creating HTML this way - can you be sure the HTML from the API will always be safe to use? Are there any considerations to make here?
| const hasImg = show.image?.medium; | ||
| image.src = hasImg ? show.image.medium : ""; | ||
| image.alt = hasImg | ||
| ? `Scene from ${show.name}` |
There was a problem hiding this comment.
Is this an appropriate use of an alt tag? Does https://axesslab.com/alt-texts/ have any useful hints?
| : `No images available for ${show.name}`; | ||
|
|
||
| card.querySelector("[data-show-summary]").innerHTML = show.summary | ||
| ? show.summary.replace(/<p>/gi, "").replace(/<\/p>/gi, "") |
There was a problem hiding this comment.
What is your thinking in replacing the
here?
This branch has not been deployed
3 participants
Learners, PR Template
Self checklist
Changelist
completed Tv-project up to level 500