Skip to content

Solution#1748

Open
Tanya-sktr wants to merge 3 commits intomate-academy:masterfrom
Tanya-sktr:develop
Open

Solution#1748
Tanya-sktr wants to merge 3 commits intomate-academy:masterfrom
Tanya-sktr:develop

Conversation

@Tanya-sktr
Copy link
Copy Markdown

No description provided.

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 your current implementation causes the Cypress end‑to‑end tests for the gallery to fail, even though the visual behavior looks correct. Specifically, in src/scripts/main.js where you handle the gallery click event (the listener on the <ul> and the code that updates the large preview image), you are very likely reading src/href via DOM properties (e.g. thumbnail.src or link.href), which convert the relative paths into absolute URLs. The tests expect the main image (and any related link) to keep a relative path like /images/landscape-first.png, but your code is setting them to http://localhost:3001/images/landscape-first.png, causing the two Gallery application Cypress tests to fail. To fix this, update that handler so it uses getAttribute('src') / getAttribute('href') from the clicked thumbnail and then calls setAttribute('src', value) / setAttribute('href', value) on the preview element, instead of using the .src / .href properties. As small robustness improvements (not blockers), consider: (1) using a more specific selector than document.querySelector('ul') for the gallery container, (2) scoping the active-thumbnail lookup to the gallery element (e.g. gallery.querySelector('.thumb-active')), and (3) adding early if (!gallery || !preview) return; guards before attaching listeners. Your event delegation approach and the click behavior itself are nicely implemented, so once you adjust how you copy the src/href values, your solution should both work correctly and pass the tests.


✨ 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

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 one core requirement of the gallery behavior is still missing: updating the preview’s surrounding anchor href so that it stays in sync with the main image and remains a relative path, which will cause the Cypress gallery tests to fail. In src/scripts/main.js, inside the gallery click handler where you already compute bigSrc and call preview.setAttribute('src', bigSrc), you also need to locate the link that wraps the preview image (for example const previewLink = preview.closest('a')) and, if it exists, call previewLink.setAttribute('href', bigSrc). This is important because the tests assert that both the large image’s src and its wrapping anchor’s href are relative paths like /images/landscape-first.png, not absolute URLs such as http://localhost:3001/images/landscape-first.png. Make sure you keep using getAttribute('href') when reading from the thumbnail, and setAttribute('href', ...) / setAttribute('src', ...) when writing, so that the values stay relative. As a minor robustness improvement, add a small guard so you only set the attribute if preview.closest('a') actually returns an element. After making this change, re-run npm test to confirm that both Cypress tests (click on small img and click outside of image) pass. You’re very close: your event delegation and relative-path handling for the main image are correct, and with this additional anchor update your solution should fully meet the task 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

e.preventDefault();

const bigSrc = link.getAttribute('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.

You correctly read the thumbnail target's href with getAttribute('href'), which preserves a relative path. However, the test suite expects that the preview's related link (the anchor that wraps the large image, if present) is updated too. After computing bigSrc you should find the preview link (for example: const previewLink = preview.closest('a')) and call previewLink.setAttribute('href', bigSrc) so the anchor's href stays a relative path as well.

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 approving your solution as an educational step so you can continue progressing while you refine the gallery behavior and better understand the requirements. You did a nice job using event delegation on the gallery list and correctly handling clicks on both the thumbnail img and its wrapping a element, which matches the core interaction goal.

Key issues to improve (critical):

  • In src/scripts/main.js, inside the gallery click handler where you derive the big image URL (near the code that uses targetImg.parentNode.href and targetLink.href), you’re reading .href values, which converts relative paths to absolute URLs and breaks the requirement to keep paths relative [CHECKLIST ITEM #4]; instead, read the link using getAttribute('href') and consider using targetImg.closest('a') rather than parentNode for robustness.
  • In the same handler where you currently do largeImg.src = bigPhoto, assigning to .src also turns the path into an absolute URL and you’re not updating the large preview’s wrapping anchor at all; you should set the preview image source with largeImg.setAttribute('src', bigSrc) and also find its closest wrapping a (e.g., const previewLink = largeImg.closest('a')) and, if it exists, update its href with setAttribute('href', bigSrc) [CHECKLIST ITEM #3, #14, #15].

Minor improvement:

  • Add a small guard like if (previewLink) { ... } before updating the preview anchor so your code won’t throw if the markup ever changes and the image is no longer wrapped in a link.

Your solution is approved in this educational sense, and with these adjustments you’ll be in a strong position to satisfy the tests and future gallery tasks.


✨ 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 targetLink = e.target.closest('a');

if (targetImg) {
const bigPhoto = targetImg.parentNode.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.

Reading the thumbnail URL via targetImg.parentNode.href returns an absolute URL. Use the thumbnail link's getAttribute('href') (for example const thumbLink = targetImg.closest('a'); const bigSrc = thumbLink.getAttribute('href');) so the value stays a relative path as required by the tests.

Comment thread src/scripts/main.js

largeImg.src = bigPhoto;
} else if (targetLink) {
const bigPhoto = targetLink.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.

Similarly, targetLink.href yields an absolute URL. Use targetLink.getAttribute('href') to read the relative path when the click target is the link itself.

Comment thread src/scripts/main.js
Comment on lines +15 to +19
largeImg.src = bigPhoto;
} else if (targetLink) {
const bigPhoto = targetLink.href;

largeImg.src = bigPhoto;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Setting largeImg.src = bigPhoto will convert the value to an absolute URL. Use largeImg.setAttribute('src', bigSrc) and also locate the preview's wrapping anchor (e.g. const previewLink = largeImg.closest('a')) and, if it exists, call previewLink.setAttribute('href', bigSrc) — include a small guard so you only set it when closest('a') returns an element.

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