Skip to content
This repository was archived by the owner on May 14, 2024. It is now read-only.

Homework week1#10

Closed
Wael-Alhomsi wants to merge 1 commit into
HackYourFuture:masterfrom
Wael-Alhomsi:homework-week1
Closed

Homework week1#10
Wael-Alhomsi wants to merge 1 commit into
HackYourFuture:masterfrom
Wael-Alhomsi:homework-week1

Conversation

@Wael-Alhomsi
Copy link
Copy Markdown

No description provided.

@harditsingh
Copy link
Copy Markdown

harditsingh commented May 13, 2018

Hey Wael!
You homework looks pretty good, nice work! That said, there are a few problems that I found:

  • main(), app.js: The main() function is referring to a variable which is present outside the function, which makes the main() an impure function! You should always pass these variables as an argument instead. Also, you have put the 'covers' object inside the function which not the best thing to do.

  • Line 49, 78, app.js: The name 'header' is not the best choice for representing the element that will contain the book title. Similarly, 'image' is not the best variable name for storing the element which contains the image of a book cover.

  • style.css: I see that you have styled HTML elements h1, h2, li, img directly instead of using classes. While this is not a problem for this assignment, you might run into problems when the websites start growing in size.

  • Line 80, app.js: This isn't an issue but a note, I saw that you used underscores ('_') for the alt attribute. You don't really need to do that, the value of the alt attribute can contain spaces.

Best of luck!

@Wael-Alhomsi
Copy link
Copy Markdown
Author

Thank you very much Hardit for reviewing my assignment and for your valuable notes. I really appreciate the time you spent on this purpose!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants