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

solution #4956

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

Conversation

NataliiaVdovyna
Copy link

  • 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 smooth

  • 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.


Thank you for your hard work on this project! 😊 While you've made great progress, there are a few critical issues that need to be addressed before we can approve the pull request:

  1. SCSS Variables and Mixins: Several SCSS variables like $button-color, $background-color-special-white, $other-text-color, $margin-inline, and $main-text-color are referenced but not defined. Similarly, mixins like on-screen2, on-screen3, and on-screen4 need to be defined to ensure responsive design. Please ensure these are defined in your stylesheets.

  2. Image and Link Paths: The src attributes for images and the href attribute in the logo link need to be checked and corrected to ensure they point to the correct files relative to the HTML file location.

  3. Layout and Hover Effects: The use of fixed column widths and hover transformations might cause layout issues on smaller screens. Consider using responsive units or media queries to adjust the layout and ensure there is enough space for hover effects.

  4. Overflow Property: The overflow-x: auto on the navigation bar might cause unwanted scrollbars. Please verify if this behavior is intended.

Please address these issues, and feel free to reach out if you need any help. Once these are resolved, we can move forward with the approval. Keep up the great work! 🚀

✨ 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
Comment on lines 28 to 29
<a
href="/src/images/logo.png"

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 <a> tag should link to a webpage, not directly to an image file. Consider changing this to a valid URL or a section within the page.

src/index.html Outdated
class="header__logo-link"
>
<img
src="/src/images/logo.png"

Choose a reason for hiding this comment

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

Ensure that the src attribute for the image points to the correct path relative to the HTML file. Currently, it points to /src/images/logo.png, which may not be correct if the HTML file is in the src directory.

<section class="card__top">
<img
class="card__top-image"
src="../src/images/imac.jpeg"

Choose a reason for hiding this comment

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

Check the src attribute for the image. It uses a relative path ../src/images/imac.jpeg, which may not be correct depending on the location of the HTML file. Ensure the path is correct.

.card {
box-sizing: border-box;
width: 200px;
background-color: $background-color-special-white;

Choose a reason for hiding this comment

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

Ensure that the SCSS variable $background-color-special-white is defined in your stylesheets. If not, you need to define it or replace it with a valid color value.

margin-top: 4px;
font-size: 10px;
line-height: 14px;
color: $other-text-color;

Choose a reason for hiding this comment

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

Ensure that the SCSS variable $other-text-color is defined in your stylesheets. If not, you need to define it or replace it with a valid color value.

Comment on lines +48 to +50
color: $button-color;
background-color: $background-color-special-white;
border: 1px solid $button-color;

Choose a reason for hiding this comment

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

Ensure that the SCSS variables $button-color and $background-color-special-white are defined in your stylesheets. If not, you need to define them or replace them with valid color values.

width: 96px;

&__star {
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.

Ensure that the path ../images/star-active.svg is correct relative to the location of the compiled CSS file. If the path is incorrect, adjust it accordingly.

}

&__star:last-child {
background-image: url(../images/star.svg);

Choose a reason for hiding this comment

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

Ensure that the path ../images/star.svg is correct relative to the location of the compiled CSS file. If the path is incorrect, adjust it accordingly.

body {
margin: 0;
}
@import './utils/variables';

Choose a reason for hiding this comment

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

Ensure that the file ./utils/variables exists and contains the necessary variable definitions used across your stylesheets.

@@ -4,0 +1,8 @@
@import './utils/variables';
@import './utils/mixins';

Choose a reason for hiding this comment

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

Ensure that the file ./utils/mixins exists and contains the necessary mixin definitions used across your stylesheets.

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.

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: 7]

If the issue is within your control, please address it.
The next review attempt will automatically launch in 1 hour.
If you believe this is an error or need assistance, please contact the support team.

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 addressing the issues in your project! 🎉 You've made significant progress by defining the necessary SCSS variables and mixins, ensuring that your styles are applied correctly. The image and link paths have been corrected, and your layout is responsive, which is fantastic.

However, remember to always verify that all SCSS variables and mixins are defined to avoid any styling issues. Also, ensure that the src and href attributes are correctly set to avoid broken links or images. The overflow-x: auto on the navigation bar should be checked to ensure it doesn't cause unwanted scrollbars unless intended.

Keep up the good work, and continue to learn from these experiences. Addressing these points will help you avoid similar issues in the future and improve your coding skills. Keep pushing forward! 🚀

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

src/index.html Outdated
<!-- #region header -->
<header class="header">
<a
href="/src/images/logo.png"

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 logo link should not point to an image file. It should link to a page or section within your website. Consider changing this to a valid URL or path.

src/index.html Outdated
class="header__logo-link"
>
<img
src="/src/images/logo.png"

Choose a reason for hiding this comment

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

The src attribute for the logo image should be checked to ensure it points to the correct file relative to the HTML file location. Verify the path is correct.

<section class="card__top">
<img
class="card__top-image"
src="../src/images/imac.jpeg"

Choose a reason for hiding this comment

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

The src attribute for the card image should be checked to ensure it points to the correct file relative to the HTML file location. Verify the path is correct.

.card {
box-sizing: border-box;
width: 200px;
background-color: $background-color-special-white;

Choose a reason for hiding this comment

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

The variable $background-color-special-white is used but not defined. Please define this variable to ensure the background color is applied correctly.

margin-top: 4px;
font-size: 10px;
line-height: 14px;
color: $other-text-color;

Choose a reason for hiding this comment

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

The variable $other-text-color is used but not defined. Please define this variable to ensure the text color is applied correctly.


align-items: center;
margin-block: 24px 16px;
margin-inline: $margin-inline;

Choose a reason for hiding this comment

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

The variable $margin-inline is used but not defined. Please define this variable to ensure the margin is applied correctly.

font-size: 12px;
font-weight: 400;
line-height: 18px;
color: $other-text-color;

Choose a reason for hiding this comment

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

The variable $other-text-color is used but not defined. Please define this variable to ensure the text color is applied correctly.

display: block;
margin-inline: $margin-inline;

background-color: $button-color;

Choose a reason for hiding this comment

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

The variable $button-color is used but not defined. Please define this variable to ensure the background color is applied correctly.

font-weight: 700;
line-height: 16px;
align-content: center;
color: $background-color-special-white;

Choose a reason for hiding this comment

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

The variable $background-color-special-white is used but not defined. Please define this variable to ensure the text color is applied correctly.

Comment on lines +48 to +50
color: $button-color;
background-color: $background-color-special-white;
border: 1px solid $button-color;

Choose a reason for hiding this comment

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

The variables $button-color and $background-color-special-white are used but not defined. Please define these variables to ensure the hover styles are applied correctly.

Copy link

@VitaliyBondarenko1982 VitaliyBondarenko1982 left a comment

Choose a reason for hiding this comment

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

something went wrong with styles on demo link.
try to deploy again and check demo link before next re-request.
Screenshot 2024-12-27 at 14 28 43

if will have some questions - feel free to ask in chat.

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.

4 participants