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

Salih-w1-browsers #45

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

Conversation

rektozarius
Copy link

No description provided.

@mnvacym mnvacym self-assigned this Jan 18, 2025
Copy link

@mnvacym mnvacym left a comment

Choose a reason for hiding this comment

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

Salih, congratulations on completing all the exercises for Week 1 of the Browsers module! You've demonstrated a strong grasp of the core concepts and successfully implemented all the required functionalities. All the unit tests have passed, which is excellent. You only need to fix your catWalk function, which currently has some bugs.

  • ex1-bookList: The book list is well implemented. The books are displayed with titles, authors, images, and background colors that depend on the read status. The styling for the list items is also well done.
  • ex2-aboutMe: You have correctly updated the spans with your personal information, and the list items are styled correctly with the class list-item in your CSS file.
  • ex3-hijackLogo: The Google logo is correctly replaced with the HackYourFuture logo by modifying the src and srcset properties.
  • ex4-whatsTheTime: The time is correctly displayed and updates every second using setInterval(). The load event listener is also correctly used, but you should ensure that you assign the function, and not the result of it, to the window.onload event listener.
  • ex5-catWalk:
    • The cat animation is not working well. When the dancing cat appears in the middle, it doesn't animate. And then, when the walking cat image appears, it also doesn't animate.
    • As in ex4-whatsTheTime, you should ensure that you assign the function, not its result, to the window.onload event listener.

Important

When you use setInterval it's important to clear it as well. Because otherwise it will cause memory leaks in your application.

// TODO execute `catWalk` when the browser has completed loading the page
const timeInterval = setInterval(catWalk, 50);

window.onload = timeInterval;
Copy link

Choose a reason for hiding this comment

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

Same here, you are assigning the result of setInterval to window.onload

you could do like this:

window.onload = function () {
  timeInterval = setInterval(catWalk, 50);
};

// TODO execute `addCurrentTime` when the browser has completed loading the page
const timeInterval = setInterval(addCurrentTime, 1000);

window.onload = timeInterval;
Copy link

Choose a reason for hiding this comment

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

You are assigning the result of setInterval to window.onload rather than the function itself. While this may work, it is not the correct way to use the window.onload handler

window.onload expects a function, but you're assigning it the return value of setInterval (which is just a numeric interval ID).

so you may do something like this:

window.onload = function() {
  setInterval(addCurrentTime, 1000);
};

Copy link

@mnvacym mnvacym left a comment

Choose a reason for hiding this comment

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

Well done

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

Successfully merging this pull request may close these issues.

2 participants