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

Develop #4888

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

Develop #4888

wants to merge 5 commits into from

Conversation

VitaliiShc
Copy link

  • DEMO LINK

  • TEST REPORT LINK

  • there 2 BEM blocks card and stars each in it 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.

It looks like there are several issues with variable definitions and a critical issue with linking to an SCSS file instead of a compiled CSS file. Please address these to ensure the code meets the required standards. Keep up the good work, and once these are fixed, your PR will be ready for approval! 😊

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

src/index.html Outdated
@@ -7,12 +7,74 @@
content="width=device-width, initial-scale=1.0"
/>
<title>Product cards</title>

<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. Make sure to compile your SCSS into CSS and link to the resulting CSS file.

@@ -0,0 +1,74 @@
.card {
width: $CardWidth;

Choose a reason for hiding this comment

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

The variable $CardWidth should be defined in your variables file or imported correctly. Ensure that it is declared and accessible here.


&__title {
font-size: 12px;
font-weight: $FontMedium;

Choose a reason for hiding this comment

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

The variable $FontMedium should be defined in your variables file or imported correctly. Ensure that it is declared and accessible here.

&__code {
font-size: 10px;
line-height: 14px;
color: $ColorSecondary;

Choose a reason for hiding this comment

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

The variable $ColorSecondary should be defined in your variables file or imported correctly. Ensure that it is declared and accessible here.

&__reviews {
font-size: 10px;
line-height: 14px;
color: $ColorMainAccent;

Choose a reason for hiding this comment

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

The variable $ColorMainAccent should be defined in your variables file or imported correctly. Ensure that it is declared and accessible here.

background: $White;
outline: 1px solid $ColorBlueAccent;
outline-offset: -1px;
color: $ColorBlueAccent;

Choose a reason for hiding this comment

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

The variable $ColorBlueAccent should be defined in your variables file or imported correctly. Ensure that it is declared and accessible here.

@@ -0,0 +1,6 @@
.page {
font-family: $PrimaryFont;

Choose a reason for hiding this comment

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

The variable $PrimaryFont should be defined in the _variables.scss file as per the code style rules.

@@ -0,0 +1,6 @@
.page {
font-family: $PrimaryFont;
color: $ColorMainAccent;

Choose a reason for hiding this comment

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

The variable $ColorMainAccent should be defined in the _variables.scss file as per the code style rules.

.page {
font-family: $PrimaryFont;
color: $ColorMainAccent;
font-weight: $FontRegular;

Choose a reason for hiding this comment

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

The variable $FontRegular should be defined in the _variables.scss file as per the code style rules.

}

@for $value from 1 through 5 {
&--#{$value} :nth-child(-n + #{$value}) {

Choose a reason for hiding this comment

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

There is an extra space between &--#{$value} and :nth-child(-n + #{$value}). According to the code style rules, there should be no space between the element and the pseudo-class.

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 are several issues that need to be addressed, such as incorrect variable naming conventions, missing variable definitions, and an incorrect file reference in the HTML. Once these are fixed, your code will be in great shape! Keep up the good work and don't hesitate to reach out if you need any help. 😊

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

src/index.html Outdated
@@ -7,12 +7,74 @@
content="width=device-width, initial-scale=1.0"
/>
<title>Product cards</title>

<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 change './styles/index.scss' to './styles/index.css'.

@use '../_variables/variables' as *;

.card {
width: $CardWidth;

Choose a reason for hiding this comment

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

The variable $CardWidth should be defined in the variables.scss file. Ensure that it is correctly defined and imported.


&__title {
font-size: 12px;
font-weight: $FontMedium;

Choose a reason for hiding this comment

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

The variable $FontMedium should be defined in the variables.scss file. Ensure that it is correctly defined and imported.

&__code {
font-size: 10px;
line-height: 14px;
color: $ColorSecondary;

Choose a reason for hiding this comment

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

The variable $ColorSecondary should be defined in the variables.scss file. Ensure that it is correctly defined and imported.

&__reviews {
font-size: 10px;
line-height: 14px;
color: $ColorMainAccent;

Choose a reason for hiding this comment

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

The variable $ColorMainAccent should be defined in the variables.scss file. Ensure that it is correctly defined and imported.

background: $White;
outline: 1px solid $ColorBlueAccent;
outline-offset: -1px;
color: $ColorBlueAccent;

Choose a reason for hiding this comment

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

The variable $ColorBlueAccent should be defined in the variables.scss file. Ensure that it is correctly defined and imported.

@use '../_variables/variables' as *;

.page {
font-family: $PrimaryFont;

Choose a reason for hiding this comment

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

The variable $PrimaryFont should follow the naming convention for SCSS variables, which typically use lowercase letters and hyphens. Consider renaming it to $primary-font.


.page {
font-family: $PrimaryFont;
color: $ColorMainAccent;

Choose a reason for hiding this comment

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

The variable $ColorMainAccent should follow the naming convention for SCSS variables, which typically use lowercase letters and hyphens. Consider renaming it to $color-main-accent.

.page {
font-family: $PrimaryFont;
color: $ColorMainAccent;
font-weight: $FontRegular;

Choose a reason for hiding this comment

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

The variable $FontRegular should follow the naming convention for SCSS variables, which typically use lowercase letters and hyphens. Consider renaming it to $font-regular.

}

@for $value from 0 through 5 {
&--#{$value} :nth-child(-n + #{$value}) {

Choose a reason for hiding this comment

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

There is an extra space before the pseudo-class ':nth-child'. It should be '&--#{$value}:nth-child(-n + #{$value})' without the space to ensure proper selector formatting.

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 organizing the HTML structure and SCSS files! 🎉 While there are some improvements needed, such as correcting variable naming conventions and ensuring all variables are defined in the variables.scss file, these are excellent learning opportunities. Please review the comments to enhance your code quality. Keep up the good work and continue to learn from these experiences!

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

@use '../_variables/variables' as *;

.card {
width: $CardWidth;

Choose a reason for hiding this comment

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

The variable $CardWidth should be defined in the variables.scss file. Please ensure it is declared there.


&__title {
font-size: 12px;
font-weight: $FontMedium;

Choose a reason for hiding this comment

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

The variable $FontMedium should be defined in the variables.scss file. Please ensure it is declared there.

&__code {
font-size: 10px;
line-height: 14px;
color: $ColorSecondary;

Choose a reason for hiding this comment

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

The variable $ColorSecondary should be defined in the variables.scss file. Please ensure it is declared there.

&__reviews {
font-size: 10px;
line-height: 14px;
color: $ColorMainAccent;

Choose a reason for hiding this comment

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

The variable $ColorMainAccent should be defined in the variables.scss file. Please ensure it is declared there.


&__price {
font-size: 12px;
color: $ColorSecondary;

Choose a reason for hiding this comment

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

The variable $ColorSecondary should be defined in the variables.scss file. Please ensure it is declared there.

background: $White;
outline: 1px solid $ColorBlueAccent;
outline-offset: -1px;
color: $ColorBlueAccent;

Choose a reason for hiding this comment

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

The variable $ColorBlueAccent should be defined in the variables.scss file. Please ensure it is declared there.

@use '../_variables/variables' as *;

.page {
font-family: $PrimaryFont;

Choose a reason for hiding this comment

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

The variable $PrimaryFont should be in lowercase according to the code style rules. Please rename it to $primary-font.


.page {
font-family: $PrimaryFont;
color: $ColorMainAccent;

Choose a reason for hiding this comment

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

The variable $ColorMainAccent should be in lowercase according to the code style rules. Please rename it to $color-main-accent.

.page {
font-family: $PrimaryFont;
color: $ColorMainAccent;
font-weight: $FontRegular;

Choose a reason for hiding this comment

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

The variable $FontRegular should be in lowercase according to the code style rules. Please rename it to $font-regular.

}

@for $value from 0 through 5 {
&--#{$value} :nth-child(-n + #{$value}) {

Choose a reason for hiding this comment

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

There is an extra space before the pseudo-class ':nth-child'. It should be '&--#{$value}:nth-child(-n + #{$value})' without the space to ensure proper CSS generation.

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