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

add task solution #5667

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

add task solution #5667

wants to merge 9 commits into from

Conversation

Arsenii34
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! 😊 However, there are a few critical issues that need to be addressed before we can approve the pull request:

  1. HTML Structure and Semantics:

    • The <header> tag is incorrectly nested within the <nav> tag. This affects the semantic structure of your HTML and should be corrected by placing the <header> as a top-level element.
    • The logo image should be wrapped in a link to improve navigation and accessibility.
  2. CSS Issues:

    • There are syntax errors, such as a missing semicolon in the .container class and incorrect usage of CSS variables (e.g., #var(--blue-color) should be var(--blue-color)).
    • The a.is-active::after selector has incorrect and duplicate properties that need to be fixed.
    • The text-transform: uppercase; is applied to the body, which is not ideal and should be limited to specific elements.
  3. Task-Specific Requirements:

    • The is-active class is not applied to any link, which is a core requirement of the task.
  4. General Observations:

    • Correct the typo in the href attribute of the first list item link.
    • Ensure consistent use of CSS variables and correct any misspellings, such as conteiner to container.

Please address these issues to ensure the functionality and requirements of the task are fully met. Once these changes are made, feel free to resubmit for review. Keep up the great 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! ✨

🔄 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
<header class="conteiner">
<img src="src/images/logo.png" alt="Logotip" class="logo">
<ul>
<li><a href="#sectionnpm run lint" class="apple text">Apple</a></li>

Choose a reason for hiding this comment

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

There seems to be a typo in the href attribute of the first list item link. The value '#sectionnpm run lint' is likely incorrect and should be corrected to a valid section ID or URL, such as '#section'.

src/style.css Outdated
}
.container{
display: flex;
background-color:white

Choose a reason for hiding this comment

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

There is a missing semicolon at the end of the 'background-color' property. This could cause issues with the CSS parsing.

src/style.css Outdated
line-height: 14px;
}
a.is-active::after{
color: #var(--blue-color);

Choose a reason for hiding this comment

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

The color property is incorrectly using '#var(--blue-color)'. The '#' should be removed to correctly use the CSS variable: 'color: var(--blue-color);'.

src/style.css Outdated


.laptops{
color:#var(--blue-color);

Choose a reason for hiding this comment

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

Similar to the previous comment, the color property is incorrectly using '#var(--blue-color)'. Remove the '#' to correctly use the CSS variable: 'color: var(--blue-color);'.

src/style.css Outdated
.text{
margin-bottom: 30px;
justify-content: end;
font-family: 'Roboto' sans-serif;

Choose a reason for hiding this comment

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

There is a missing comma between 'Roboto' and 'sans-serif'. It should be 'font-family: 'Roboto', sans-serif;'.

Copy link
Author

Choose a reason for hiding this comment

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

между Roboto и sans-serif нужна просто запятая или ещё пробел

src/style.css Outdated

a.active::after {
content: '';
background-color: #var(--blue-color);

Choose a reason for hiding this comment

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

The background-color property is incorrectly using '#var(--blue-color)'. Remove the '#' to correctly use the CSS variable: 'background-color: var(--blue-color);'.

src/style.css Outdated Show resolved Hide resolved
@Arsenii34
Copy link
Author

@Arsenii34
Copy link
Author

@Arsenii34
Copy link
Author

2 similar comments
@Arsenii34
Copy link
Author

@Arsenii34
Copy link
Author

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