Skip to content

Conversation

aakash1sadhu
Copy link

  • DEMO LINK

  • TEST REPORT LINK

  • there 2 BEM blocks card and stars each in their own file

  • SCSS Nesting is used for elements, modifiers and pseudo-classes

  • SCSS Variables are used for main values and placed in a separate file

  • all stars--N modifiers work as expected (Highlight first N stars)

  • Code follows all the Code Style Rules ❗️

Copy link

@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

Decision: REQUEST_CHANGES — One critical blocker found.

Why: Your HTML image path violates the explicit requirement to "Use images only from src/images in the project." This can break automated checks and potentially the image display, so it’s a critical blocker.

Issues to fix (blocking):

  • src/index.html: Update the product image path from images/imac.jpeg to reference the src/images folder as required.
    • Likely line: the <img> tag inside the card (look for src="images/imac.jpeg").
    • Why it’s wrong: The requirement mandates that assets be referenced from src/images.
    • How to fix: Change the attribute to something like src="src/images/imac.jpeg" (or the correct path per your build setup). Ensure the actual file resides at src/images/imac.jpeg.

What’s good: Everything else looks solid—BEM structure, data-qa attributes, SCSS organization with variables/mixins, hover states, and the stars block all meet the requirements.

Once you update the image path, this should be good to approve. Great work overall—just a quick path correction to finish strong!


✨ 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

@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

Decision: REQUEST_CHANGES — automated visual tests are failing due to missing selectors and significant visual diffs, which are critical blockers.

Why this is critical:

  • The BackstopJS tests report selectors not found: Element not found for capturing: [data-qa="card"] and timeout for [data-qa="hover"]. These attributes are explicit checklist requirements and used by tests to locate elements. If they’re absent at runtime, the task cannot be validated.
  • Visual diffs for the targeted elements are >40%, far above the 1% threshold, indicating the rendered card does not match the reference closely enough.

What to fix (be specific):

  1. Ensure required data-qa attributes are present in the rendered HTML (built output), not just in source:
    • In src/index.html, the card root must include data-qa="card" on the card block element, e.g., <div class="card" data-qa="card">…</div>.
    • The BUY link must include data-qa="hover", e.g., <a class="card__buy" data-qa="hover" …>BUY</a>.
    • If using a bundler (e.g., Parcel), verify it does not strip data attributes. Open the served page in DevTools and confirm the attributes exist at runtime.
  2. Resolve visual mismatches flagged by BackstopJS:
    • Compare fonts, sizes, paddings, margins, colors, and image sizing with the reference. Small deviations can cause large diff percentages.
    • Verify the card width is exactly 200px including border (box-sizing: border-box) and that spacing matches the mockup.
    • Confirm the Roboto font is loading (check Network tab) and is applied on body/.page.
  3. Run tests locally and iterate:
    • Run npm test or backstop test locally, open the generated HTML report, and adjust styles/markup until diffs are under the 1% threshold.

Minor suggestions (optional):

  • Double-check image paths resolve in the built environment (src/images/... in HTML, ../images/... in SCSS) and that images load without 404s.

You’ve done a solid job with BEM, SCSS structure, variables, and mixins. Once the data-qa attributes are confirmed in the final HTML and the visual diffs are brought within threshold, this should pass. Keep going—you’re very close!


✨ 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

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