task solution#1773
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Changes are needed. The implementation has critical issues: First, when clicking an a element, the code fails because it tries to access e.target.src directly, but a elements don't have a src attribute—you need to find the nested img inside using e.target.querySelector('img') or e.target.closest('a').querySelector('img'). Second, the click handler is navigating to the thumbnail URL (landscape-first-thumb.{hash}.jpeg) instead of the full-size image URL (images/landscape-first.png) as expected by the tests. Make sure to get the large image URL from the correct data source (likely a data attribute on the a element or its nested img). Third, the forEach loop modifying all links' href to '#' is unnecessary and doesn't follow proper event delegation.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| link.href = '#'; | ||
| }); | ||
|
|
||
| largeImg.src = e.target.src; |
There was a problem hiding this comment.
The code doesn't handle the case when e.target is an a element. According to the requirements, event.target can be either img or a. If it's an a, you need to find the nested img inside it to get the src. You can use e.target.querySelector('img') to get the image when clicking on the link.
|
|
||
| gallery.addEventListener('click', (e) => { | ||
| listItemsLinks.forEach((link) => { | ||
| link.href = '#'; |
There was a problem hiding this comment.
The forEach loop iterating over all links and setting their href to '#' is unnecessary. Proper event delegation should use e.target to determine which element was clicked and act accordingly - no need to modify other elements.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The implementation has a critical issue with URL retrieval. The code uses e.target.closest('a').href which retrieves the thumbnail image URL, but the tests expect the full-size image URL (e.g., images/landscape-first.png). This means the gallery will display the wrong image when a thumbnail is clicked, breaking the core functionality. To fix this, add a data-source attribute to the anchor element containing the full-size image path (like data-source="images/landscape-first.png"), then update the click handler to use e.target.closest('a').dataset.source instead of .href. Additionally, consider adding a null check for closest('a') to prevent errors if the click target isn't inside an anchor element.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| gallery.addEventListener('click', (e) => { | ||
| e.preventDefault(); | ||
|
|
||
| const link = e.target.closest('a').href; |
There was a problem hiding this comment.
This retrieves the a element's href, which typically points to the thumbnail image. The tests expect the full-size image URL (e.g., images/landscape-first.png). Consider using a data attribute on the a element (like data-source="images/landscape-first.png") and accessing it with e.target.closest('a').dataset.source instead.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This is an educational override to help you progress while learning. You demonstrated solid event delegation with closest('a'), and the core gallery concept is working. However, there's a critical null safety bug on lines 10-14: you're accessing linkElement.setAttribute() and linkElement.href before the null check on line 14. When clicking outside an anchor element, closest('a') returns null, and these property accesses will throw a TypeError. Move the null check to line 10, right after closest('a'), to fix this. Additionally, the data-link attribute approach adds unnecessary complexity — you can directly use largeImg.src = linkElement.href. These are great learning opportunities about defensive coding and simplifying code. Keep it up!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
|
|
||
| linkElement.setAttribute('data-link', linkElHRef); | ||
|
|
||
| if (!linkElement) { |
There was a problem hiding this comment.
The null check is in the wrong position. Lines 10 and 12 access linkElement properties before the check on line 14, which will throw an error when clicking outside an anchor element.
| const linkElement = e.target.closest('a'); | ||
| const linkElHRef = e.target.closest('a').href; | ||
|
|
||
| linkElement.setAttribute('data-link', linkElHRef); |
There was a problem hiding this comment.
Setting a data-link attribute on every click is unnecessary. Directly use linkElement.href to set the large image source.
| return; | ||
| } | ||
|
|
||
| largeImg.src = linkElement.getAttribute('data-link'); |
There was a problem hiding this comment.
Reading from getAttribute('data-link') instead of directly using href adds unnecessary complexity. Simplify to largeImg.src = linkElement.href.
danon321
left a comment
danon321
left a comment
There was a problem hiding this comment.
Zapodaj jeszcze DEMO LINK do tego zadanka zeby mozna było sprawdzic czy działa to poprawnie
3 participants
No description provided.