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

isa-w1-Browsers #47

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

isaerenie
Copy link

No description provided.

@isaerenie isaerenie changed the title Browsers w1 assignment is comleted isa-w1-Browsers Jan 9, 2025
@tlorent tlorent self-assigned this Jan 13, 2025
@tlorent tlorent self-requested a review January 13, 2025 10:11
Copy link

@tlorent tlorent left a comment

Choose a reason for hiding this comment

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

Great work @isaerenie 👏 Code already looks good and I see that you've put in quite some effort for this, nice job! Some of the exercises are not working completely as expected yet, please see if you can fix this 💪

Comment on lines +25 to +28
liElement.style.border = '1px solid black';
liElement.style.margin = '10px';
liElement.style.padding = '10px';
liElement.style.listStyle = 'none';
Copy link

Choose a reason for hiding this comment

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

I would suggest to not set these styles via JavaScript, you can just use the CSS file for this 🙂

Comment on lines +37 to +38
imgElement.style.width = '100px';
imgElement.style.height = '150px';
Copy link

Choose a reason for hiding this comment

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

The same for these styles 😄 You can add them in the CSS file.

Copy link

Choose a reason for hiding this comment

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

Almost! I see the design does not completely match with the expected outcome 😄 Can you see if you can fix this? 💪

Screenshot 2025-01-13 at 11 43 31

Copy link

Choose a reason for hiding this comment

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

}

updateTime();
setInterval(updateTime, 1000);
Copy link

Choose a reason for hiding this comment

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

Great code! Works as expected 💪 One tip when it comes to setting intervals, I think it's a good practice to also clear them to avoid memory leaks and performance problems.

https://vaidehijoshi.github.io/blog/2015/01/06/the-final-countdown-using-javascripts-setinterval-plus-clearinterval-methods/

By clearing the interval you make sure that it does not run anymore when it is not required.

Copy link

Choose a reason for hiding this comment

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

I see the cat is walking nicely across the screen but unfortunately it keeps moving in the middle of the screen with the other image 😄 Please see if you can fix this 💪

Screenshot 2025-01-13 at 11 45 29

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

Successfully merging this pull request may close these issues.

2 participants