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

review requested #1

Open
wants to merge 49 commits into
base: for-review
Choose a base branch
from
Open

review requested #1

wants to merge 49 commits into from

Conversation

Junitalama
Copy link
Owner

No description provided.

@Junitalama Junitalama requested review from ButcherDing and AnnaFYZ May 4, 2023 11:02
@netlify
Copy link

netlify bot commented May 4, 2023

Deploy Preview for cyf-junitalama-tv ready!

Name Link
🔨 Latest commit 6354dc5
🔍 Latest deploy log https://app.netlify.com/sites/cyf-junitalama-tv/deploys/6470fc5981d23e0008edea33
😎 Deploy Preview https://deploy-preview-1--cyf-junitalama-tv.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 settings.

Copy link
Collaborator

@AnnaFYZ AnnaFYZ left a comment

Choose a reason for hiding this comment

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

good job!!!!

script.js Outdated
@@ -1,12 +1,100 @@
//You can edit ALL of the code here
const allEpisodes = getAllEpisodes();
function setup() {
const allEpisodes = getAllEpisodes();
Copy link
Collaborator

Choose a reason for hiding this comment

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

you have declared allEpisodes above, so i think you can skip this line of code as it is repeated

script.js Outdated
divEle.append(
episodeName,
lineEle,
image,
Copy link
Collaborator

Choose a reason for hiding this comment

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

i learn from you too :) i didn't know you can add them all at one go, cool!!

let options = document.createElement("option");
options.value = el.name;
options.innerText = `${el.name} - S${el.season.toString().padStart(2, "0")}E${el.number.toString().padStart(2, "0")}`;

Copy link

Choose a reason for hiding this comment

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

wise approach for padStart()

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great job getting everything working up to level-300!

I think it was optional anyway, but the only thing missing in terms of functionality is being able to easily go back to seeing all episodes after using the dropdown - the only way to get them back right now is to start typing and then delete again.

I think in terms of style/readability, it's good to break up code into some smaller sub-functions - it can make the code much easier to read (because usually a function says what it does), and easier for you to think about too. There is a lot happening in these functions! And the 300 level bit could be put ("encapsulated") in a function.

Copy link

@GergelyKI GergelyKI left a comment

Choose a reason for hiding this comment

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

Looks good, I left a few comments

script.js Outdated
Comment on lines 180 to 188
const searchInput = searchEle.value.toLowerCase();
const filteredEpisodes = allEpisodes.filter(episode => {
if (episode.name.toLowerCase().includes(searchInput) || episode.summary.toLowerCase().includes(searchInput)){
return episode;
}
})
document.querySelector("#num").innerText = filteredEpisodes.length;
makePageForEpisodes(filteredEpisodes);
})

Choose a reason for hiding this comment

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

Here the indentation looks a bit flat, like the last 3 lines start at the same line, but they are not at the same scope level.

"program": "${workspaceFolder}/script.js"
}
]
}

Choose a reason for hiding this comment

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

VS code settings are usually not committed, you could add this file to your gitignore.

@Junitalama Junitalama requested a review from ButcherDing May 26, 2023 18:26
@migmow
Copy link

migmow commented May 26, 2023

Great job!💯🏅
There're some shows without having any images to display such as "The Jefferson". Can you think of handling this scenario?

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.

6 participants