Skip to content
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

Errors #4855

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Errors #4855

wants to merge 8 commits into from

Conversation

QuantumCoder228
Copy link

No description provided.

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.

Great job on your submission! 🎉 While the code is well-structured and effectively handles various interactions, there are a few areas for improvement to enhance functionality and user experience:

  1. BEM and SCSS Usage: Ensure all class names follow BEM methodology consistently across all files.
  2. Data Attributes: Add data-qa="card-hover" to the "Buy" link inside the first card as specified.
  3. Hover Effects: Consider using a more subtle scale value to prevent layout shifts.
  4. Dynamic Star Ratings: Implement logic to dynamically render the correct number of filled stars based on the reviews value.
  5. Image Handling: Add error handling or a default image for product.imgSrc.
  6. Navigation Links: Provide valid URLs or implement JavaScript functions for navigation links and the logo.
  7. 'Buy' Button: Implement a proper link or event handler for purchasing functionality.

These improvements will help you meet the task requirements and constraints more effectively. Keep up the good work, and continue to learn from these insights! 🚀

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

@@ -27,7 +27,9 @@ module.exports = async (page, scenario) => {
}

if (postInteractionWait) {
await new Promise(resolve => setTimeout(resolve, postInteractionWait));
await new Promise(resolve => {
setTimeout(resolve, postInteractionWait);

Choose a reason for hiding this comment

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

Ensure that postInteractionWait is a positive integer to avoid unexpected behavior in the setTimeout function.

card.setAttribute('data-qa', 'card');

card.innerHTML = `
<img class="card__img" src="${product.imgSrc}" alt="${product.altText}" />

Choose a reason for hiding this comment

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

Consider adding error handling or a default image in case product.imgSrc is not a valid URL or the image fails to load.

<span class="stars__star"></span>
<span class="stars__star"></span>
</div>
<p class="card__text-review">Reviews: ${product.reviews}</p>

Choose a reason for hiding this comment

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

The number of stars displayed does not currently reflect the product.reviews value. Consider implementing logic to dynamically render the correct number of filled stars based on the reviews value.

<p class="card__text-price">Price:</p>
<p class="card__text-value">${product.price}</p>
</div>
<a href="#" class="card__button">Buy</a>

Choose a reason for hiding this comment

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

The 'Buy' button currently has a href attribute set to '#', which doesn't lead anywhere. Consider implementing a proper link or event handler for purchasing functionality.

src/index.html Outdated
<h1>Catalog</h1>
<header class="header">
<a
href="#"

Choose a reason for hiding this comment

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

The href attribute for the logo link is set to '#', which doesn't lead anywhere. Consider providing a valid URL or implementing a JavaScript function to handle the click event if navigation is intended.

color: $normal-color;

&:hover {
color: $active-color;

Choose a reason for hiding this comment

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

Ensure that $active-color is defined in the imported variables file. If not, this could lead to a compilation error or unexpected styling.

}

&.is-active {
color: $active-color;

Choose a reason for hiding this comment

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

Ensure that $active-color is defined in the imported variables file. If not, this could lead to a compilation error or unexpected styling.

}

.is-active {
color: $active-color;

Choose a reason for hiding this comment

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

Ensure that $active-color is defined in the imported variables file. If not, this could lead to a compilation error or unexpected styling.

position: absolute;
bottom: 0;
left: 0;
background-color: $active-color;

Choose a reason for hiding this comment

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

Ensure that $active-color is defined in the imported variables file. If not, this could lead to a compilation error or unexpected styling.

Comment on lines 17 to 18
&:nth-child(-n + 4) {
background-image: url(../images/star-active.svg);

Choose a reason for hiding this comment

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

The :nth-child(-n + 4) selector will always apply the active star image to the first four stars. If the number of active stars should be dynamic based on a rating value, consider using JavaScript to dynamically apply a class or style to the appropriate number of stars.

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