-
-
Notifications
You must be signed in to change notification settings - Fork 2
(COMPLETED) WM5 | Marcus Zagorski | TV-Show-Library | #4
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. |
adniyaYousaf
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.
Well done Marcus! I like your TV show layout and design.
adniyaYousaf
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.
I think it would be more user-friendly if there was a Home button available to easily navigate back to the main page after selecting a show from the list. Otherwise, it looks great!!
nirmeet-baweja
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.
Well done! Its great to see your progress in the course and to see it reflected in your code as well. 🎉
| <link rel="icon" href="/favicon.ico" type="image/x-icon" /> | ||
| </head> | ||
|
|
||
| <body> |
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.
I would suggest giving your app a name or a main heading. So that when a user navigates to your website, they know the goal / purpose of the website. But nice page design and layout! Well done there.
| function setup() { | ||
| const allEpisodes = getAllEpisodes(); | ||
| makePageForEpisodes(allEpisodes); | ||
| const currentState = { |
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.
It is a nice idea to use a single object to keep track of all the states that you need.
| let layoutSelector = document.querySelector(".layout"); | ||
|
|
||
| // Fetch API function to fetch data upon call | ||
| async function fetchAPI(url) { |
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.
Good job on breaking down the code into small functions.
| } | ||
|
|
||
| // Fetches shows data via API | ||
| async function getAllShows() { |
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 happens if the code fails to fetch the shows? Is the user informed about the failed action in any way?
| } | ||
|
|
||
| async function getAllEpisodes() { | ||
| try { |
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.
Same comment as above. Is the user kept updated with the state of the website?
| function createEpisodeCard(episode) { | ||
| const layoutSelector = document.querySelector(".layout"); | ||
|
|
||
| const linkCardsToTVMaze = createElement("a", ""); |
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.
Is it good use of semantic html to have a section tag within ana tag?
| } | ||
| } | ||
|
|
||
| function dropDownAfterSearch(currentShows) { |
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.
Is this function being used?
| searchLayout.appendChild(displayingFilterResults); | ||
| } | ||
|
|
||
| async function searchFilterEpisodes() { |
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.
I was unable to use the filter episodes feature. When I navigate to the episodes page for a certain show and type something in the search field, the page takes me back to the search results for shows instead.
| addCardLayout(); | ||
| clearCards(); | ||
| makePageForEpisodes(currentState.allEpisodes); | ||
| currentState.episodesFetched = false; |
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.
Why is currentState.episodesFetched set to false after the episodes are being fetched?
| rootElem.textContent = `Got ${episodeList.length} episode(s)`; | ||
| async function render() { | ||
| try { | ||
| await getAllShows().then((allEpisodes) => { |
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.
Are we working with a list of shows or with a list of episodes in this part of the code?
Here is my complete TV Show-Library. I hope you enjoy 😊