Skip to content

js_gallery_DOM#1746

Open
gadyka08-lab wants to merge 3 commits intomate-academy:masterfrom
gadyka08-lab:develop
Open

js_gallery_DOM#1746
gadyka08-lab wants to merge 3 commits intomate-academy:masterfrom
gadyka08-lab:develop

Conversation

@gadyka08-lab
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, although your src/scripts/main.js correctly implements the gallery behavior (event delegation on #thumbs, handling clicks on both <img> and <a>, preventing default navigation, and updating #largeImg.src from the clicked link’s href), this pull request modifies several files that the task explicitly said must not be changed. The assignment requires you to change only src/scripts/main.js, but your PR also includes edits to README.md, package.json, package-lock.json, and the .github/workflows configuration, which violates that core checklist requirement and needs to be fixed before the solution can be accepted. Please revert or remove all changes outside src/scripts/main.js, so that only the gallery implementation remains in this task’s commit. While you are adjusting the README, also double-check that the demo link actually uses your GitHub Pages URL instead of a placeholder like <your_account>. As a minor (optional) improvement, you could defensively check that document.getElementById('thumbs') and document.getElementById('largeImg') are not null before using them, to avoid errors if the HTML structure changes. Once the unrelated file changes are reverted and the task constraint is satisfied, your main.js implementation looks solid for this assignment.


✨ 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

Changes are needed before this can be accepted, even though your gallery logic in src/scripts/main.js is functionally correct and meets the behavior requirements (event delegation on #thumbs, handling clicks on both the <img> and its <a>, preventing default navigation, and updating the large image’s src/alt).

The critical blocker is that the assignment explicitly requires modifying only src/scripts/main.js, but this PR also includes edits in other files such as README.md, package.json, and possibly others; all changes outside src/scripts/main.js must be reverted or removed from this PR so that it matches the task constraints. Once you isolate the PR to just the main.js changes, your implementation of the gallery will satisfy the functional checklist items. As an additional improvement (not a blocker), double-check that your README demo link actually uses your GitHub username instead of the <your_account> placeholder, and make sure you have run the required npm test commands locally so you can catch any hidden issues.

Your core solution is solid; the main fix is to clean up the extra file changes so the submission aligns precisely with the stated 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

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.

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]

If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.

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 override so you can keep progressing while you address a few remaining points. You did a strong job in src/scripts/main.js by using event delegation on #thumbs, correctly handling clicks on both the thumbnail <img> and its parent <a>, preventing the default navigation, and updating the large image’s src and alt attributes as required [CHECKLIST ITEM #1, #2, #3, #4, #9, #10, #11].

There are a couple of critical issues to fix: the assignment requires that only src/scripts/main.js be modified, but your pull request also changes other files (such as README.md, package.json, or workflow files), so please revert those extra edits so that the PR touches only src/scripts/main.js [CHECKLIST ITEM #12, #13]. In src/scripts/main.js, around the code where you currently set largeImg.alt = link.title; (roughly lines 20–30), add a fallback like largeImg.alt = link.title || linkedImg?.alt || ''; so that the large image still has meaningful alt text when title is missing, which improves accessibility [CHECKLIST ITEM #4].

Minor improvements: make sure the README demo link uses your actual GitHub username instead of the placeholder and confirm you have run npm run test, npm run test:only -- -n, and npm run test:only -- -l, addressing any failures [CHECKLIST ITEM #5, #6, #7, #8, #14, #15]. Overall, your core gallery logic is well done; with these cleanups and the small accessibility enhancement, your solution will be both correct and polished.


✨ 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
@@ -1 +1,40 @@
'use strict';
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Critical: the assignment requires modifying ONLY src/scripts/main.js. Verify your pull request contains no other file changes (README, package.json, etc.). If other files were changed, revert those edits so the PR touches only this file (checklist item #13).

Comment thread src/scripts/main.js
// додатково оновлюємо 'alt' для доступності,
// використовуючи заголовок посилання

largeImg.alt = link.title;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Consider an accessibility fallback here: if link.title is empty or not provided, use the thumbnail's alt (e.g., link.querySelector('img')?.alt) so the large image alt isn't left blank.

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