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

Roland's Project Cinema (week 3) #28

Open
wants to merge 27 commits into
base: master
Choose a base branch
from

Conversation

rolandjlevy
Copy link

Hi Dmitri

Here is my Project Cinema again but on the master branch this time.

Thanks,
Rol

@@ -0,0 +1 @@
Roland Levy,rolandjlevy,Rolands-MacBook-Pro.local,21.09.2018 10:07,file:///Users/rolandjlevy/Library/Application%20Support/OpenOffice/4;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like it should be going into .gitignore or be deleted

let loaded = 0;
let currentID = "";

function getMoviesFromSearch (url){
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like these functions do the same thing. Can we not re-use one of them?

clearNodes();
return;
}
return filmList.map(film => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice

resultsPagesNode.innerHTML = html;

const pageButtonList = document.querySelectorAll(".page-number-link");
pageButtonList.forEach(pageButton => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

would be better to use delegation here

const input = event.target.value;
if (input === "") {
loaded = 0;
return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we return here we don't need an else. That helps to keep code more linear

}
}
})
resultsHeaderNode.innerHTML = `<div class="results-showing">Please start by typing in your search...</div>`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be better to move this to the top. It feels a little lost here

return fetch(url)
.then(response => response.json())
.then(filmObject => {
if (window.innerWidth < 768) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would this not be better handled using CSS? It seems like we have different code more desktop and iPad.

}).catch(error => console.log(error));
}

Array.prototype.filterOut = function(ignore) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's bad practice extending the prototype of native JavaScript objects. https://developers.google.com/web/updates/2018/03/smooshgate

@dmitrigrabov
Copy link
Collaborator

I was looking forward to your README

@dmitrigrabov
Copy link
Collaborator

Good work otherwise :)

@rolandjlevy
Copy link
Author

Hi Dmitri

I just completed the README file.

Thanks for all your comments. I have some questions about them. Would be good to clarify.

Thanks,
Rol

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