-
-
Notifications
You must be signed in to change notification settings - Fork 2
WM5| AHMADZAY BAHADORY| TV-SHOW| completed level 200| Added search input and dropdown menu with functionality. #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for cyf-marcuszagorski-tv ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
@MarcusZagorski |
MarcusZagorski
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall some very good code Bahadory. Keep up the good work! I've noted a few points that can help you
| let getEpi = getAllEpisodes(); | ||
|
|
||
| function getEpiName(getEpi) { | ||
| for (let i = 0; i < getEpi.length; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice for loop. Do you think a for each loop would be more beneficial in this scenario?
| episode.name.toLowerCase().includes(searchTerm) | ||
| ); | ||
|
|
||
| layoutSelector.innerHTML = ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use a function that you've already created to handle this?
| defaultOption.textContent = "Please select..."; | ||
| episodeSelect.appendChild(defaultOption); | ||
|
|
||
| episodes.forEach((episode) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forEach is an excellent choice! Well done!
| }); | ||
|
|
||
| episodeSelect.addEventListener("change", (event) => { | ||
| const selectedEpisodeName = event.target.value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done! I like that you've used a variable here to specify what the event.target.value is
|
|
||
| if (selectedEpisodeName === "Please select...") { | ||
| clearRender(); | ||
| const allEpisodes = getAllEpisodes(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think getAllEpisodes() needs to be declared again? If so why?
| }); | ||
| } | ||
|
|
||
| episodeSelector.addEventListener("input", function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we should use "input" or "change" as the event listener? Look up what the difference is between them
| <body> | ||
| <div id="root"></div> | ||
| <div class="input-wrapper"> | ||
| <input type="search" id="episodeSearch" placeholder="Search Episodes" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What other tag do we need for this input type to help with accessibility?
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.