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

implemented MOYO header #5365

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

Conversation

vickyme16
Copy link

@vickyme16 vickyme16 commented Sep 15, 2024

DEMO LINK
TEST REPORT LINK

  • [ x ] Header height is set in 1 place (for the links)
  • [ x ] Content is vertically centered (for any header height)
  • [ x ] CSS is used to show all letters in Uppercase (don't type them in HTML)
  • [ x ] Logo is an image wrapped with a link
  • [ x ] CSS Variable is used for a blue color
  • [ x ] Pseudo-element is used for a blue line below the active link
  • [ x ] 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.

Great job on the Pull Request! 🌟 While there are some suggestions for improvement, such as using classes for styling instead of tag names, providing fallback fonts, adding empty lines for readability, fixing href values, and improving alt text, these are not critical issues and don't significantly impact the functionality or security of your code. Keep these tips in mind for future projects to enhance your code quality! 👍

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

src/index.html Outdated Show resolved Hide resolved
Comment on lines +37 to +38
href="apple"
>

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 anchor tag should start with a '#' symbol when linking to an anchor on the same page. Also, avoid using spaces in the href value.

src/index.html Outdated Show resolved Hide resolved
src/index.html Outdated Show resolved Hide resolved
src/index.html Outdated Show resolved Hide resolved
src/index.html Outdated Show resolved Hide resolved
src/style.css Outdated Show resolved Hide resolved
src/style.css Outdated Show resolved Hide resolved
src/style.css Outdated Show resolved Hide resolved
src/style.css Outdated Show resolved Hide resolved
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.

you need to provide the links from readme.md file to the pull request description

don't forget to implement all these steps
image

@vickyme16
Copy link
Author

you need to provide the links from readme.md file to the pull request description

don't forget to implement all these steps image

Done, thanks!

src/style.css Outdated
Comment on lines 35 to 52
nav a:hover {
color: var(--primary-color);
}

nav {
display: flex;
align-items: center;
height: 60px;
}

nav ul {
list-style: none;
padding: 0;
margin: 0;
display: flex;
height: 100%;
align-items: center;
}

Choose a reason for hiding this comment

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

image fix it everywhere

src/style.css Outdated
Comment on lines 67 to 69
content: ''; /* Псевдоелемент не додає текст */
display: block; /* Робимо псевдоелемент блоковим */
width: 100%; /* Ширина підкреслювання відповідає ширині посилання */

Choose a reason for hiding this comment

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

remove all comments. Try to write obvious code that explains all we need without comments)

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 👍

display: flex;
justify-content: space-between;
width: auto;
background-color: white;

Choose a reason for hiding this comment

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

This color is default

Suggested change
background-color: white;

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