Skip to content

solution#1744

Open
aakash1sadhu wants to merge 1 commit intomate-academy:masterfrom
aakash1sadhu:develop
Open

solution#1744
aakash1sadhu wants to merge 1 commit intomate-academy:masterfrom
aakash1sadhu:develop

Conversation

@aakash1sadhu
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

Your solution is approved because it correctly implements the required gallery behavior: clicking a thumbnail image or its surrounding link updates the main large image using event delegation on the gallery list. The handler in src/scripts/main.js properly distinguishes between IMG and A targets and sets largeImg.src accordingly, so all core functional requirements for the task are met.

There are a couple of small robustness improvements you can make next time you revisit this code: on the line where you currently call e.preventDefault() unconditionally in the click handler, consider moving that call inside the branches that handle IMG and A so other click types keep their default behavior. Similarly, in the IMG branch, you do e.target.closest('a').href; it would be safer to store const anchor = e.target.closest('a') and check that anchor is not null before reading anchor.href, in case the markup changes and an image is no longer wrapped in a link. As a low-priority accessibility enhancement, you could also update largeImg.alt based on the clicked thumbnail’s alt so screen readers get the correct description when the main image changes.

Overall, this is a clear, focused implementation that meets the task goals, and the remaining suggestions are minor improvements you can treat as learning opportunities for future projects.


✨ 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 largeImg = document.querySelector('#largeImg');

list.addEventListener('click', (e) => {
e.preventDefault();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Calling e.preventDefault() unconditionally prevents the default action for all clicks inside the list (including non-anchor clicks). Consider moving preventDefault() into the branches that handle anchor clicks (or only call it when the click target or its closest ancestor is an a) so other clicks don't get their default behavior blocked.

Comment thread src/scripts/main.js
e.preventDefault();

if (e.target.tagName === 'IMG') {
largeImg.src = e.target.closest('a').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.

When handling the IMG case you call e.target.closest('a').href without checking that closest('a') actually found an anchor. Add a null-check (or guard) before accessing .href to avoid potential runtime errors if the markup changes.

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