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

create a files catalog #4913

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

Conversation

olga-wukelan
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.


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:

  1. Variable Typos: There's a typo in the SCSS variable name $font-weigth-bold. It should be $font-weight-bold. This needs to be corrected to ensure styles are applied correctly.

  2. Missing Variable Definitions: Ensure that variables like $main-color, $primary-color, and $secondary-color are defined in your variables file. Missing definitions can lead to compilation errors.

  3. Incorrect Selector: The selector &__button:hover is incorrect. It should be &:hover to apply the hover effect on the button itself.

  4. File Path Issues: Verify that all image paths and SCSS import paths are correct. Incorrect paths can lead to missing styles or images.

  5. SCSS Compilation: The stylesheet is linked as index.scss, which browsers cannot use directly. Ensure this SCSS file is compiled to CSS and the correct CSS file is linked.

Once these issues are resolved, your code will be in great shape! Keep up the great work, and feel free to reach out if you have any questions. 😊

✨ 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
@@ -17,11 +18,360 @@
/>
<link
rel="stylesheet"
href="styles/index.scss"
href="./index.scss"

Choose a reason for hiding this comment

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

The stylesheet is linked as index.scss, which is not directly usable by browsers. Ensure that this SCSS file is compiled to CSS and the correct CSS file is linked here.

src/index.html Outdated
>
<img
class="card__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 image source path ../src/images/imac.jpeg might not be correct depending on your file structure. Ensure the path is correct relative to the HTML file location.

src/index.html Outdated Show resolved Hide resolved
src/index.html Outdated Show resolved Hide resolved
src/index.html Outdated Show resolved Hide resolved

.is-active {
position: relative;
color: $main-color;

Choose a reason for hiding this comment

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

Ensure that the variable $main-color is defined in your variables file. If it's not defined, this will cause a compilation error.

display: block;
content: '';
border-radius: 8px;
background-color: $main-color;

Choose a reason for hiding this comment

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

Ensure that the variable $main-color is defined in your variables file. If it's not defined, this will cause a compilation error.

margin-right: 4px;
background-repeat: no-repeat;
background-position: center;
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 image path './images/star.svg' is correct and the file exists at the specified location.

src/styles/stars.scss Outdated Show resolved Hide resolved
src/styles/variables.scss Outdated Show resolved Hide resolved
Copy link

@volodymyr-soltys97 volodymyr-soltys97 left a comment

Choose a reason for hiding this comment

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

Good job 👍
Let's improve your code

src/index.html Outdated
Comment on lines 121 to 123
<h1 class="card__title">
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A)
</h1>

Choose a reason for hiding this comment

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

This tag used for title of the full page, consider will be better if you use the another tag here. Fix it everywhere

Suggested change
<h1 class="card__title">
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A)
</h1>
<h2 class="card__title">
APPLE A1419 iMac 27" Retina 5K Monoblock (MNED2UA/A)
</h2>

Copy link
Author

Choose a reason for hiding this comment

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

я не розуміюю. у мене це ж і є основним заголовком до карти. чи мається на увазі, що всі карти це другорядні на сторінці і може бути заголовок до блоку каталогу? @volodymyr-soltys97

src/index.html Outdated Show resolved Hide resolved
src/index.html Outdated Show resolved Hide resolved
src/styles/blocks/card.scss Outdated Show resolved Hide resolved
src/styles/index.scss Outdated Show resolved Hide resolved
@@ -1,3 +1,13 @@
body {
margin: 0;
font-family: Roboto, sans-serif;
font-weight: 400;
color: #060b35;

Choose a reason for hiding this comment

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

Suggested change
color: #060b35;

Copy link

@etojeDenys etojeDenys left a comment

Choose a reason for hiding this comment

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

good job

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