-
Notifications
You must be signed in to change notification settings - Fork 0
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
London10-Afsha_Hossain_TV_dom_project #1
base: for-review
Are you sure you want to change the base?
Conversation
…and using the script.js to get the data from episode.js file Restructured the index.html and changed the names a bit. We moved on from the using html to create the webpage. Instead converted the html elements to JS by creating variables in JS and appended the variables inside other variables so that it represents the our html tree. Also made changes to the pseudocode and css. Added the source in the footer. To complete level 100, I need to remove the <p> from the episode descriptions and also made the episodeInfoCards fit in the cardsContainer. And probably several other things...
…r to fix the layout
Changed .episode-name-num-holder width size to 20rem in css.
Correcting the last comment in script.js file
Need to find a way to stop the description text going out of the episodeInfoHolder.
Just need to make every the episode container size same.
Added some divs using JS particularly around images to have a cage for wild images. Added a bit of pseudocode for level 200. Corrected the previous h1 and h2 elements to span and p elements. Created input element for the search bar.
✅ Deploy Preview for cyf-afsha10-tv1 ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for cyf-afsha10-tvv ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
Made the episodes page responsive by using % for width instead of rem
Made the show page responsive by using the flexbox instead of grid to arrange the elements inside the show cards. To use the flexbox, I created a new div container called "showImageSummaryInfoContainer" and I've put three other elements inside this container in a row.
…n css and changed variable names Made some the 4 small info description bold using CSS, gave comma and spacing between different genres using .split and .join and changed some variable names.
…ch bar Created a function to be able to search shows in the search bar. It is same as episodes. I only changed the episodes to shows.
…ng between shows and episodes Fixed switching appearing of unnecessary dropdown menus while switching between shows and episodes by hiding visibility and allowing visibility.
Corrected the infinite loop by taking the eventlistener in line 211 outside the for loop
Created 2 separate search bars- one for episodes and the other for shows. W make them visible and hide them same like we did with episode and show dropdowns in makePageForEpisodes and makePageForShows
Cleared the search input entered in show page and episode by episodeSearchInputHtml.value = "" where makePageForShow is called and by showSearchInputHtml.value = "" where makePageForEpisode is called.
…ave created it in JS.
|
||
episodeSearchResultDisplayHtml.style.display = "inline"; | ||
showSearchResultDisplayHtml.style.display = "none"; // hide episode search result from show page | ||
|
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 looks like you're doing the same thing 6 times which is very repetitive. Is there possibly a good word to possibly describe what we're doing. Is there a function name that we can use, using that word.
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.
Thank you very much for pointing this out! Is the word to display and hide called called "toggle"?
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.
Hi Afsha,
That is up to you. What I would say is that you should try it and see how it goes with "toggle()" and if you need any more help let myself or one of the team know :)
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 might be able to manage this one. There are a lot of other bug I need to fix. It feels overwhelming sometimes. Thank you for you so much for your kind offer!
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.
No problem! Trust me I am still learning as well so do not worry one bit. A bit of advice I can give in terms of feeling overwhelmed is to try and tell someone or write down al the problems you are having as you can tend to find the answer while you're speaking about the problem.
i hope this helps and let me know if you need anything else!
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.
Hey Faisal,
I have made some changes to reduce some repetition by using a toggleVisibility function.
For me there is nothing like speaking about the problem. It gives me clarity. However, rarely I get to speak about the coding problems. I have been making checklists about the all the coding related problems in a notebook and have made some progress. Thank you for the reminder!
I am currently struggling a few small assignments related to fetch and API and we are starting React. Would you be interested in to pair programming sometime?
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.
Hi Afsha,
I apologise for the late reply. I did not see this until today when I logged into Gituhub. Yes I would be happy to start pair programming. I am not super strong in programming as I have not done much since I finished my degree however, I still practice in my free time. I would be happy to do it. Let me know when you want to do it.
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.
Hey Faisal!
No worries about the delay. Before we start pair programming, it would be great if you could join our in-person training sessions on Saturdays. It'll give us a chance to meet face-to-face and discuss pair programming and our projects in more detail.
Let me know if you're interested, and I'll share the details of the upcoming Saturday session. You can also find the information in our Slack channel. Feel free to check it out.
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.
Hi Afsha,
Yes I will be more than happy to come in person for a session. I won't be able to attend tomorrow as I am away for the weekend however nest Saturday works perfectly for me. Could you please forward me the details and I will see you then!
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.
Tomorrow it will be remote. Next week it will probably be at Lego, which is exciting! Feel free to DM on slack or here if you need the address.
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.
This is generally looking really solid, nicely put together!
One usability thing I noticed using your page - if you filter by searching in the text box, the page moves around a bit - can you work out why this is? Can you think how to improve this?
script.js
Outdated
episodeList.forEach((episode, index) => { | ||
// Get the episode name, season number, episode number, and combine them | ||
const episodeName = episode.name; | ||
const paddedSeasonNum = ("0" + episode.season).slice(-2); |
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.
Rather than repeating this code on this and the next line, maybe we could extract a function with a clear name which shows what it's doing?
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.
Thank you for pointing this out! I created 2 different functions to format the name in the episode dropdown list and to format episode titles in episode cards. I had to create two functions because they are formatted slightly differently.
script.js
Outdated
// Display selectedEpisode | ||
rootHtml.innerHTML = ""; | ||
makePageForEpisodes(selectedEpisode); | ||
// buildEpisodeDropdownList(selectedShows); |
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'd recommend deleting the commented code, it makes it easier to follow what's actually going on (and easier to review!)
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.
Apologies for that. I have removed those comments and the unnecessary console logs.
|
||
// creating a status element and its value inside showBasicInfoContainerHtml | ||
|
||
const showStatusValueHtml = document.createElement("span"); |
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'm seeing a pattern of a lot of "document.createElement ; element.classList.add ; element.innerHTML =" (and also then appendChild
) - is there a useful function to extract here which does these three things? It may make it more clear to see what's going on.
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.
You are right about the pattern. Will it be something like createElementAddClassAppend function?
I'm not sure about the innerHTML. Sometimes we use .innerHTML and sometimes we use .src.
…ow and episode elements Used a function to show and hide episode and show elements when the show and episode pages are displayed
…les in the episodeCards and episodeTitles in episodeDropdownMenu Created 2 different functions to format the name in the episode dropdown list and also episode titles in episode cardsRemoved unnecessary comments and console.logs.
Fixed the episode and show pages to appear at the top the of page by using this code: window.scrollTo(0, 0) ;
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.
This is looking really well put together - good job!
const paddedSeasonNum = ("0" + episode.season).slice(-2); | ||
const paddedEpisodeNum = ("0" + episode.number).slice(-2); |
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.
This looks a lot like you're implementing the same thing in this function and in getFormattedEpisodeNameInDropdown
- is there a third function you could extract, which you could call from both of those, to avoid having to write this twice?
const rootElem = document.getElementById("root"); | ||
rootElem.textContent = `Got ${episodeList.length} episode(s)`; | ||
// make the episode dropdown list visible and hide the show dropdown list | ||
toggleVisibility(episodeSelectHtml, showSelectHtml); |
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.
"toggle" implies "swap" (i.e. that if you call toggle once it will change something, and call it again it will change it back) - I think what you're really doing in toggleVisibility
is hiding the elements - can you think of a more clear name for the function?
|
||
const episodeNameNumSeasonComboHolderHtml = document.createElement("div"); | ||
const episodeNameNumSeasonComboTextHtml = document.createElement("span"); | ||
episodeNameNumSeasonComboHolderHtml.classList.add( |
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 looks like every element you create, you add a class-name for - can you maybe extract a function which would mean you could write something like:
const episodeNameNumSeasonComboHolderHtml = createElement("div", "episode-name-num-holder");
instead of keep having to the same things over and over?
Are there more things you're repeatedly doing you could also extract into this function, as well?
Level 300 WIP