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

Card #4876

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

Card #4876

Conversation

SemenVodolazskij
Copy link

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.

The issue with linking to the SCSS file instead of the compiled CSS file needs to be addressed as it impacts the functionality of the code. Please make this change and resubmit. Keep up the great work! 😊

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

src/index.html Outdated
href="https://fonts.googleapis.com/css2?family=Roboto:wght@400;500;700&display=swap"
rel="stylesheet"
/>

<link
rel="stylesheet"
href="./styles/index.scss"

Choose a reason for hiding this comment

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

The href attribute in the link tag should point to a CSS file, not an SCSS file. Please ensure you are linking to the compiled CSS file instead of the SCSS source file.

@@ -1,3 +1,95 @@
body {
margin: 0;
font-family: Roboto, sans-serif, Arial;

Choose a reason for hiding this comment

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

The font-family declaration should list the generic family name 'sans-serif' last, after specific font names like 'Roboto' and 'Arial'. This ensures that if none of the specified fonts are available, a generic sans-serif font will be used.

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.

It looks like there's a critical issue with linking the SCSS file directly, which can affect the functionality of the webpage. Please compile the SCSS into CSS and update the link to ensure everything works smoothly. Keep up the great work, and you'll have this sorted in no time! 😊

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

src/index.html Outdated
href="https://fonts.googleapis.com/css2?family=Roboto:wght@400;500;700&display=swap"
rel="stylesheet"
/>

<link
rel="stylesheet"
href="./styles/index.scss"

Choose a reason for hiding this comment

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

The stylesheet link references a SCSS file (index.scss). Typically, browsers do not directly understand SCSS files, so you should compile your SCSS into CSS and link to the resulting CSS file instead.

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 the HTML structure! While there are some issues related to class naming conventions and accessibility, these are excellent learning opportunities. Please review the comments to enhance your code quality. Remember, addressing these points will help you avoid similar issues in the future. Keep up the good work and continue improving! 🚀

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

Comment on lines +43 to +46
<div class="stars_active"></div>
<div class="stars_active"></div>
<div class="stars_active"></div>
<div class="stars_active"></div>

Choose a reason for hiding this comment

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

It seems like you have multiple divs with the class 'stars_active'. If these are meant to represent individual stars in a rating system, consider using a more semantic approach, such as using a list or a span with appropriate ARIA attributes for better accessibility.

@@ -0,0 +1,95 @@
body {
margin: 0;
font-family: Roboto, sans-serif, Arial;

Choose a reason for hiding this comment

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

The font-family property should list generic font families at the end. Consider placing 'Arial' before 'sans-serif'.

background-image: url(../images/imac.jpeg);
}

.product_code {

Choose a reason for hiding this comment

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

Class names should use hyphens instead of underscores according to the BEM naming convention. Consider renaming '.product_code' to '.product-code'.

line-height: 14px;
}

.star_review {

Choose a reason for hiding this comment

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

Class names should use hyphens instead of underscores according to the BEM naming convention. Consider renaming '.star_review' to '.star-review'.

justify-content: space-between;
}

.reviews {

Choose a reason for hiding this comment

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

Class names should use hyphens instead of underscores according to the BEM naming convention. Consider renaming '.reviews' to '.review'.

margin-right: 4px;
}

.stars {

Choose a reason for hiding this comment

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

Class names should use hyphens instead of underscores according to the BEM naming convention. Consider renaming '.stars' to '.star'.

margin-right: 4px;
}

.price {

Choose a reason for hiding this comment

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

Class names should use hyphens instead of underscores according to the BEM naming convention. Consider renaming '.price' to '.pricing'.

margin-bottom: 16px;
}

.text_price {

Choose a reason for hiding this comment

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

Class names should use hyphens instead of underscores according to the BEM naming convention. Consider renaming '.text_price' to '.text-price'.

color: #616070;
}

.numeric_price {

Choose a reason for hiding this comment

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

Class names should use hyphens instead of underscores according to the BEM naming convention. Consider renaming '.numeric_price' to '.numeric-price'.

font-weight: 700;
}

.button_buy {

Choose a reason for hiding this comment

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

Class names should use hyphens instead of underscores according to the BEM naming convention. Consider renaming '.button_buy' to '.button-buy'.

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