Skip to content
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

COMPLETE THE LEVEL 500 #1

Open
wants to merge 31 commits into
base: main
Choose a base branch
from
Open

COMPLETE THE LEVEL 500 #1

wants to merge 31 commits into from

Conversation

adniyaYousaf
Copy link
Owner

@adniyaYousaf adniyaYousaf commented Feb 7, 2024

  • I have committed my files one by one, on purpose, and for a reason
  • I have titled my PR with COHORT_NAME | FIRST_NAME LAST_NAME | REPO_NAME | WEEK
  • I have tested my changes
  • My changes follow the style guide
  • My changes meet the requirements of this task

This comment was marked as duplicate.

Copy link

netlify bot commented Feb 11, 2024

Deploy Preview for adniyayoussaf-tvshow ready!

Name Link
🔨 Latest commit 773106b
🔍 Latest deploy log https://app.netlify.com/sites/adniyayoussaf-tvshow/deploys/6691a10f3e6a2a0008620eed
😎 Deploy Preview https://deploy-preview-1--adniyayoussaf-tvshow.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

@nirmeet-baweja nirmeet-baweja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have done a great job! 👏🏽 You have good attention to detail and nice grasp of the logical aspects of breaking down the problem into smaller chunks.

dist/css/pngtree-home-icon-png-png-image_4513200.png Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Show resolved Hide resolved
})
}
// fetch Api function to fetch the Episodes of selected show
async function getAllEpisodes() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is generally considered to be good practice to provide the external data needed by the function as an argument. For example, it would be better to pass selectedShow as an argument rather than relying directly on the global variable. The global variable can be modified elsewhere in the code while the function is still in the middle of running and can lead to unexpected behaviour in certain cases.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to pass selectedShow as argument but somehow it was't working. I will look into it again. Thank you.

script.js Outdated Show resolved Hide resolved
clearShows();
const episodeName = selectedOption.textContent.split('-')
SearchTerm = episodeName[1];
makePageCards();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please elaborate how would the function makePageCards act in the scope of this current function?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I select a episode from the episode dropdown I only see a black screen, is that the intended outcome or is it a bug?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makePageCards create filtered cards for Episodes acoording to SearchTerm. This function is Updating the SearchTerm and calling the makePageCards function to create filtered cards.

Yes, there is bug in Episodes dropDown. I will try to fix it. Thank you

Copy link

netlify bot commented Jul 12, 2024

Deploy Preview for relaxed-bunny-77ee37 ready!

Name Link
🔨 Latest commit 773106b
🔍 Latest deploy log https://app.netlify.com/sites/relaxed-bunny-77ee37/deploys/6691a10f5081e50008f202d5
😎 Deploy Preview https://deploy-preview-1--relaxed-bunny-77ee37.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants