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 #4880

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

Develop #4880

wants to merge 4 commits into from

Conversation

koguts
Copy link

@koguts koguts commented Oct 16, 2024

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 critical issues related to missing units in CSS properties, which can significantly impact the layout and styling of the application. Please address these to ensure the code functions as expected. Keep up the great work, and once these are fixed, your code will be even better! 😊

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

body {
margin: 0;
padding: 20;

Choose a reason for hiding this comment

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

The padding value should have a unit. Consider using 'px' or another appropriate unit for the padding value.

flex-direction: column;
align-items: center;
padding: 32px 16px 16px;
border: 1 solid #f3f3f3;

Choose a reason for hiding this comment

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

There is a missing unit for the border width. It should be specified, for example, as '1px solid #f3f3f3'.

align-items: center;
padding: 32px 16px 16px;
border: 1 solid #f3f3f3;
border-radius: 5;

Choose a reason for hiding this comment

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

The border-radius value is missing a unit. It should be specified, for example, as '5px'.

border-radius: 5;

&__image {
height: 134;

Choose a reason for hiding this comment

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

The height value is missing a unit. It should be specified, for example, as '134px'.

&__image {
height: 134;
width: 160;
margin-bottom: 40;

Choose a reason for hiding this comment

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

The margin-bottom value is missing a unit. It should be specified, for example, as '40px'.

height: 40;
border-radius: 5;
border: 1 solid #00acdc;
font-size: 14;

Choose a reason for hiding this comment

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

The font-size value is missing a unit. It should be specified, for example, as '14px'.

border-radius: 5;
border: 1 solid #00acdc;
font-size: 14;
line-height: 16;

Choose a reason for hiding this comment

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

The line-height value is missing a unit. It should be specified, for example, as '16px'.

display: flex;

&__star {
height: 16;

Choose a reason for hiding this comment

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

There is a missing unit for the height property. It should be specified, for example, as '16px'.

@@ -0,0 +1,9 @@
@mixin font {

Choose a reason for hiding this comment

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

The mixin name 'font' is too generic. Consider using a more descriptive name to indicate what the mixin is specifically for, such as 'roboto-font'.

font-family: Roboto, serif;
}

@mixin display-position {

Choose a reason for hiding this comment

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

The mixin name 'display-position' could be more descriptive. Consider renaming it to something like 'flex-center' to better convey its purpose.

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 related to CSS properties that need to be addressed. These include incorrect font-weight values and missing units for font-size and margin-bottom. Additionally, adhering to the recommended code style for using 0 instead of 0px would improve the code quality. Please make these changes to ensure the code meets the required standards. Keep up the good work, and you're almost there! 😊

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

body {
margin: 0;
margin: 0px;

Choose a reason for hiding this comment

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

According to the Code Style Rules ❗️, it is recommended to use 0 instead of 0px for the margin property.

// }

&__title {
font-weight: 500px;

Choose a reason for hiding this comment

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

There is a typo in the font-weight value. It should be a number (e.g., 500) instead of '500px'.

&__code {
color: #616070;
font-weight: 400;
font-size: 10;

Choose a reason for hiding this comment

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

The font-size value is missing a unit. It should be specified in 'px', 'em', etc.

}

&__reviews-value {
font-weight: 400px;

Choose a reason for hiding this comment

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

There is a typo in the font-weight value. It should be a number (e.g., 400) instead of '400px'.

}

&__price-value {
font-weight: 700;

Choose a reason for hiding this comment

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

The font-size value is missing a unit. It should be specified in 'px', 'em', etc.

font-weight: 700;
font-size: 16;
line-height: 18px;
margin-bottom: 16;

Choose a reason for hiding this comment

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

The margin-bottom value is missing a unit. It should be specified in 'px', 'em', etc.

border: 1px solid #00acdc;
font-size: 14px;
line-height: 16px;
font-weight: 700px;

Choose a reason for hiding this comment

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

There is a typo in the font-weight value. It should be a number (e.g., 700) instead of '700px'.

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 structuring your SCSS files and using BEM methodology! 🎉 While there are a few minor issues with the 'font-weight' values, which should be numeric (e.g., '700' instead of '700px'), your overall approach is solid. Please review the comments to improve your code. Keep up the good work and continue learning from these experiences!

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

// }

&__title {
font-weight: 500px;

Choose a reason for hiding this comment

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

The value for 'font-weight' should be a numeric value, not '500px'. It should be '500'.


&__code {
color: #616070;
font-weight: 400px;

Choose a reason for hiding this comment

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

The value for 'font-weight' should be a numeric value, not '400px'. It should be '400'.

}

&__reviews-value {
font-weight: 400px;

Choose a reason for hiding this comment

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

The value for 'font-weight' should be a numeric value, not '400px'. It should be '400'.


&__price-title {
color: #616070;
font-weight: 400px;

Choose a reason for hiding this comment

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

The value for 'font-weight' should be a numeric value, not '400px'. It should be '400'.

border: 1px solid #00acdc;
font-size: 14px;
line-height: 16px;
font-weight: 700px;

Choose a reason for hiding this comment

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

The value for 'font-weight' should be a numeric value, not '700px'. It should be '700'.

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