-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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 #4833
base: master
Are you sure you want to change the base?
Develop #4833
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.
Thank you for your hard work on this project! There are a few critical issues that need to be addressed before we can approve the pull request. Specifically, the navigation links all point to 'index.html', which may not be the intended behavior if they are supposed to navigate to different sections or pages. Additionally, the 'card__price_sum' divs are empty, which might not be intentional. Please ensure these elements are correctly implemented to meet the task requirements. Once these issues are resolved, your submission will be in great shape! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review
button in the Reviewers tab.
src/index.html
Outdated
<li class="nav__item"> | ||
<a | ||
class="nav__link is-active" | ||
href="index.html" |
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.
All navigation links point to 'index.html'. If these links are intended to navigate to different pages or sections, ensure the 'href' attributes are set correctly.
src/index.html
Outdated
<li class="nav__item"> | ||
<a | ||
class="nav__link" | ||
href="index.html" |
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.
All navigation links point to 'index.html'. If these links are intended to navigate to different pages or sections, ensure the 'href' attributes are set correctly.
src/index.html
Outdated
<li class="nav__item"> | ||
<a | ||
class="nav__link" | ||
href="index.html" |
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.
All navigation links point to 'index.html'. If these links are intended to navigate to different pages or sections, ensure the 'href' attributes are set correctly.
src/index.html
Outdated
<li class="nav__item"> | ||
<a | ||
class="nav__link" | ||
href="index.html" |
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.
All navigation links point to 'index.html'. If these links are intended to navigate to different pages or sections, ensure the 'href' attributes are set correctly.
src/index.html
Outdated
<li class="nav__item"> | ||
<a | ||
class="nav__link" | ||
href="index.html" |
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.
All navigation links point to 'index.html'. If these links are intended to navigate to different pages or sections, ensure the 'href' attributes are set correctly.
line-height: 38px; | ||
|
||
&:hover { | ||
color: v.$hover-color; |
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.
Ensure that the variable 'v.$hover-color' is defined in the 'variables' module. If it's not defined, this will cause a compilation error.
|
||
&__link { | ||
text-decoration: none; | ||
color: varia.$main-text-color; |
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.
Ensure that the variable 'varia.$main-text-color' is defined in the 'variables' module. If it's not defined, this will cause a compilation error.
transition: color 300ms; | ||
|
||
&:hover { | ||
color: varia.$hover-color; |
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.
Ensure that the variable 'varia.$hover-color' is defined in the 'variables' module. If it's not defined, this will cause a compilation error.
|
||
.is-active { | ||
margin-left: 0; | ||
color: varia.$hover-color; |
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.
Ensure that the variable 'varia.$hover-color' is defined in the 'variables' module. If it's not defined, this will cause a compilation error.
height: 4px; | ||
width: 100%; | ||
border-radius: 5px; | ||
background-color: varia.$hover-color; |
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.
Ensure that the variable 'varia.$hover-color' is defined in the 'variables' module. If it's not defined, this will cause a compilation error.
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.
Thank you for your submission! Your project is well-structured and demonstrates a good understanding of HTML and SCSS. However, there are a few areas that need attention:
-
HTML File: The
card__price_sum
divs are still empty. Please ensure these elements are populated with the correct price information as per the task requirements. -
SCSS Files:
- Ensure that the variable
v.$hover-color
is correctly defined in the 'variables' module, as it is crucial for the button border and background color. - Similarly, ensure that the variable
v.$price-in-string
is defined to display the price correctly. - The
&__title
selector is nested under the:hover
pseudo-class. If the intention is to change the title color on hover, this is correct. Otherwise, consider adjusting its placement.
- Ensure that the variable
Despite these issues, your work is on the right track. The decision to approve is made to help you move forward with your learning. Please review the comments and address the issues 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! ✨
|
||
<div class="card__price"> | ||
<div class="card__price_title">Price:</div> | ||
<div class="card__price_sum"></div> |
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.
The 'card__price_sum' div is empty. Ensure that this element is populated with the correct price information as per the task requirements.
|
||
<div class="card__price"> | ||
<div class="card__price_title">Price:</div> | ||
<div class="card__price_sum"></div> |
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.
The 'card__price_sum' div is empty. Ensure that this element is populated with the correct price information as per the task requirements.
|
||
<div class="card__price"> | ||
<div class="card__price_title">Price:</div> | ||
<div class="card__price_sum"></div> |
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.
The 'card__price_sum' div is empty. Ensure that this element is populated with the correct price information as per the task requirements.
|
||
<div class="card__price"> | ||
<div class="card__price_title">Price:</div> | ||
<div class="card__price_sum"> </div> |
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.
The 'card__price_sum' div is empty. Ensure that this element is populated with the correct price information as per the task requirements.
|
||
<div class="card__price"> | ||
<div class="card__price_title">Price:</div> | ||
<div class="card__price_sum"></div> |
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.
The 'card__price_sum' div is empty. Ensure that this element is populated with the correct price information as per the task requirements.
|
||
<div class="card__price"> | ||
<div class="card__price_title">Price:</div> | ||
<div class="card__price_sum"></div> |
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.
The 'card__price_sum' div is empty. Ensure that this element is populated with the correct price information as per the task requirements.
|
||
<div class="card__price"> | ||
<div class="card__price_title">Price:</div> | ||
<div class="card__price_sum"></div> |
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.
The 'card__price_sum' div is empty. Ensure that this element is populated with the correct price information as per the task requirements.
&__title { | ||
color: #34568b; | ||
} |
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.
The '&__title' selector is nested under the ':hover' pseudo-class, which means the title color change will only apply when the card is hovered. If the intention is to change the title color on hover, this is correct. Otherwise, consider moving it outside the hover block.
|
||
&_sum { | ||
&::after { | ||
content: '$#{v.$price-in-string}'; |
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.
Ensure that the variable 'v.$price-in-string' is correctly defined in the 'variables' module. This variable is used to display the price, and if it's not defined, it will cause issues.
box-sizing: border-box; | ||
width: 100%; | ||
height: 40px; | ||
border: 1px solid v.$hover-color; |
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.
Ensure that the variable 'v.$hover-color' is correctly defined in the 'variables' module. This variable is used for the button border and background color, and if it's not defined, it will cause issues.
DEMO LINK
TEST REPORT LINK
All components follow BEM and use SCSS
Repeated sizes and special colors are put to variables
Grid is used for the columns
Cards are shown in 1, 2, 3, or 4 columns based on screen resolution
All changes on
:hover
are smoothCode follows all the Code Style Rules ❗️