-
Notifications
You must be signed in to change notification settings - Fork 17
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
rizan_ibrahim-browsers-w1-assingment #44
base: main
Are you sure you want to change the base?
rizan_ibrahim-browsers-w1-assingment #44
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rizan, great job on completing the exercises for Week 1 of the Browsers module! You've made good progress and implemented the required functionalities for the assignments. Most of the unit tests have passed, which shows a good understanding of the core concepts.
- ex1-bookList: The book list needs improvements. Images are not showing up at the moment. Try to build file paths for
src
dynamically by using relative path. You also need to add thealt
attribute to the images, as indicated by the test failure. - 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
andsrcset
properties. - ex4-whatsTheTime: The time is correctly displayed and updates every second using
setInterval()
. But it triggers every 100 ms. The load event listener is correctly used. - ex5-catWalk: The cat animation is working well, and the dancing cat appears in the middle before the original image is restored.
.test-summary/TEST_SUMMARY.md
Outdated
|
||
| Exercise | Passed | Failed | ESLint | | ||
|------------------|--------|--------|--------| | ||
| ex1-bookList | 5 | 1 | ✕ | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Warning
You have one failing test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Ammar i didnt know exactly what was the mistake here but i will see it thanks for check
const infoBook = document.createElement('p'); | ||
infoBook.textContent = `${book.title} by ${book.author}`; | ||
const imgBook = document.createElement('img'); | ||
imgBook.src = `https://via.placeholder.com/100x150?text=${encodeURIComponent(book.title)}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to use https://via.placeholder.com
?
Can't you just achieve it via a relative path? You can use the relevant book title and build the file name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well u are right but i just want to try this way for practice since the traditional way is moe common than this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand, but when I run the app locally, book images are not showing up. Can you fix it?
} | ||
window.addEventListener('load', () => { | ||
addCurrentTime(); | ||
setInterval(addCurrentTime, 100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip
You trigger the addCurrentTime
function every 100 milliseconds. Although the timer shows the correct time, it runs every 100 milliseconds, not every second (1000 milliseconds). This may negatively affect your app's performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh my bad i will fix it sure thanks for check
// TODO complete this function | ||
} | ||
const cat = document.querySelector('img'); | ||
let position = parseInt(cat.style.left, 10) || 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice usage of parseInt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I run the app locally, book images are not showing up. Could you fix it?
No description provided.