Skip to content

add task solution#1741

Open
bodyafell wants to merge 2 commits intomate-academy:masterfrom
bodyafell:develop
Open

add task solution#1741
bodyafell wants to merge 2 commits intomate-academy:masterfrom
bodyafell:develop

Conversation

@bodyafell
Copy link
Copy Markdown

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Review Summary

I am requesting changes because the current implementation does not meet two core requirements of the task and will likely fail the automated checks. In src/scripts/main.js, in the section where you select all thumbnail elements and call forEach(... addEventListener ...), you attach individual listeners to each thumbnail instead of using a single delegated listener on their common parent; the task explicitly requires event delegation so that one handler on the container handles all thumbnail clicks [CHECKLIST ITEM #3]. In the same area, the handler logic relies on the thumbnail variable (e.g., using thumbnail.firstElementChild.href) rather than inspecting event.target, which means clicks on the img versus the surrounding a are not handled in the robust way the task requires [CHECKLIST ITEM #2]. To fix this, attach one click listener to the thumbnails container, use event.target.closest('a') inside that handler to find the clicked link, and use link.getAttribute('href') to determine the large image URL. Also, call event.preventDefault() only when a valid anchor was actually clicked so normal browser behavior isn't blocked unnecessarily. As a minor improvement, you can add simple null checks for the large image element and the container before adding the listener to avoid runtime errors if the markup changes. Once you update the code to use event delegation and event.target/closest() as described, your solution should align well with the requirements.


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/scripts/main.js Outdated
@@ -1 +1,11 @@
'use strict';

const thumbnails = document.querySelectorAll('.list-item');
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Selecting all thumbnails and adding separate listeners prevents using event delegation. The task explicitly requires event delegation (checklist item #3). Instead attach a single listener to the thumbnails container (a common parent) and determine which thumbnail was clicked via event.target/closest().

Comment thread src/scripts/main.js Outdated
Comment on lines +6 to +7
thumbnails.forEach((thumbnail) => {
thumbnail.addEventListener('click', (evt) => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The forEach + addEventListener approach here means you're not using event.target. The requirement states a click can be on the small img or on the a (checklist item #2), so the handler must inspect event.target (and/or use event.target.closest('a')) to get the correct href for the large image.

Comment thread src/scripts/main.js Outdated
thumbnails.forEach((thumbnail) => {
thumbnail.addEventListener('click', (evt) => {
evt.preventDefault();
largeImage.src = thumbnail.firstElementChild.href;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This line reads thumbnail.firstElementChild.href instead of using the event object. That won't reliably work when event.target is the img (or when markup differs). Use event.target (or event.target.closest('a')) and getAttribute('href') to obtain the image URL, and call evt.preventDefault() only when an anchor click should be prevented.

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Review Summary

Your solution is approved because it correctly implements the required gallery behavior: a single delegated click listener on the thumbnails container updates the large image when either a thumbnail img or its surrounding a is clicked, and event.preventDefault() is only called for valid thumbnail links. In src/scripts/main.js, the overall event delegation pattern and use of event.target with closest(...) match the task’s requirements and ensure clicks on nested elements are handled properly. To make the code more robust, consider adding a guard before thumbnails.addEventListener(...) (e.g., if (!thumbnails || !largeImage) return;) so the script won’t throw if the expected DOM elements are missing. It would also be more future-proof to use event.target.closest('a') rather than a class-based selector, so the logic depends on the anchor element itself instead of a specific CSS class. Finally, using link.getAttribute('href') when setting largeImage.src will preserve any relative paths instead of converting them to absolute URLs via the .href property. Overall, the core functionality is solid; these suggestions are minor improvements to enhance resilience and maintainability.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

Comment thread src/scripts/main.js
const thumbnails = document.querySelector('.gallery__list');
const largeImage = document.querySelector('.gallery__large-img');

thumbnails.addEventListener('click', (evt) => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If thumbnails is null, thumbnails.addEventListener(...) will throw. Add a guard (e.g. if (!thumbnails || !largeImage) return;) before attaching the listener so the script is safe if the DOM structure changes or the elements are missing.

Comment thread src/scripts/main.js
const largeImage = document.querySelector('.gallery__large-img');

thumbnails.addEventListener('click', (evt) => {
const currentThumbnail = evt.target.closest('.list-item__link');
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You're using a class selector in closest(...). Consider using event.target.closest('a') (or verifying the found element is an anchor) so the handler doesn't depend on a specific CSS class and correctly matches anchor elements as required by the task.

Comment thread src/scripts/main.js

evt.preventDefault();

largeImage.src = currentThumbnail.href;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Using currentThumbnail.href returns an absolute URL; to preserve the original (relative) href value use currentThumbnail.getAttribute('href') when assigning largeImage.src.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants