-
Notifications
You must be signed in to change notification settings - Fork 5.1k
add task solution #5923
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
add task solution #5923
Conversation
There was a problem hiding this 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 thesrc/images
folder as required.- Likely line: the
<img>
tag inside the card (look forsrc="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 atsrc/images/imac.jpeg
.
- Likely line: the
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
There was a problem hiding this 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):
- 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 includedata-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.
- In
- 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
.
- Run tests locally and iterate:
- Run
npm test
orbackstop test
locally, open the generated HTML report, and adjust styles/markup until diffs are under the 1% threshold.
- Run
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.
DEMO LINK
TEST REPORT LINK
there 2 BEM blocks
card
andstars
each in their own fileSCSS Nesting is used for
elements
,modifiers
andpseudo-classes
SCSS Variables are used for main values and placed in a separate file
all
stars--N
modifiers work as expected (Highlight firstN
stars)Code follows all the Code Style Rules ❗️