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.

Sign up for GitHub

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

Jump to bottom

moyo_header_develop #103

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

moyo_header_develop #103

wants to merge 7 commits into from

Conversation

Lazarend
Copy link

@Lazarend Lazarend commented Feb 15, 2023 •
edited
Loading

DarkMistyRoom
DarkMistyRoom suggested changes Feb 16, 2023
View reviewed changes
Copy link

@DarkMistyRoom DarkMistyRoom left a comment

Choose a reason for hiding this comment

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

add the demo and test links to the PR description. and push the readme file

src/index.html Outdated
Comment on lines 25 to 26
</a>
<nav class="nav">
Copy link

Choose a reason for hiding this comment

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

Add empty lines between multiline sibling blocks of HTML

Copy link

Choose a reason for hiding this comment

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

fix it everywhere

src/index.html Outdated
Comment on lines 38 to 43
<a
href="#"
class="nav__link"
data-qa="hover">
Laptops & Computers
</a>
Copy link

Choose a reason for hiding this comment

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

Suggested change
<a
href="#"
class="nav__link"
data-qa="hover">
Laptops & Computers
</a>
<a
href="#"
class="nav__link"
data-qa="hover"
>
Laptops & Computers
</a>

src/index.html Outdated
Comment on lines 46 to 47
<a href="#"
class="nav__link">Gadgets</a>
Copy link

Choose a reason for hiding this comment

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

Suggested change
<a href="#"
class="nav__link">Gadgets</a>
<a
href="#"
class="nav__link"
>
Gadgets
</a>

Copy link

Choose a reason for hiding this comment

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

fix it everywhere

Copy link
Author

Choose a reason for hiding this comment

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

Hi! Ok, already fixed everywhere.

Volodymyr-Melnyk-Fron10der
Volodymyr-Melnyk-Fron10der suggested changes Feb 27, 2023
View reviewed changes
Copy link

Choose a reason for hiding this comment

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

image
your demo link shows smth wrong here(

src/index.html Outdated
<body>

<h1>Moyo header</h1>
Copy link

Choose a reason for hiding this comment

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

no space between parent and child nodes

Suggested change
<body>
<h1>Moyo header</h1>
<body>
<h1>Moyo header</h1>

Copy link

Choose a reason for hiding this comment

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

check it throughout your code

src/index.html Outdated

<header class="header" id="home">

<a href="#home" class="logo__link">
Copy link

Choose a reason for hiding this comment

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

looks like logo is an element of the header

Suggested change
<a href="#home" class="logo__link">
<a href="#home" class="header__logo">

src/style.css Outdated
@@ -1 +1,64 @@
/* Styles go here */
Copy link

Choose a reason for hiding this comment

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

remove all comments

src/style.css Outdated
body {
margin: 0;
font-family: "Roboto", Arial, sans-serif;
--main-color: #00acdc;
Copy link

Choose a reason for hiding this comment

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

I suggest to define variables in :root pseudo selector

Copy link
Author

Choose a reason for hiding this comment

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

Hi. Ok, thanks I fixed everything)

etojeDenys
etojeDenys reviewed Nov 27, 2023
View reviewed changes
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.

image also feel free to ask for some in the chat help with links(you can always tag me there)

src/index.html
Comment on lines +57 to +60
<a href="#"
class="menu-link">
Gadgets
</a>
Copy link

Choose a reason for hiding this comment

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

Suggested change
<a href="#"
class="menu-link">
Gadgets
</a>
<a href="#" class="menu-link">
Gadgets
</a>

if you have only two attributes you can write it in one line

src/index.html
Comment on lines +50 to +54
<a href="#"
class="menu-link "
data-qa="hover">
Laptops & Computers
</a>
Copy link

Choose a reason for hiding this comment

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

Suggested change
<a href="#"
class="menu-link "
data-qa="hover">
Laptops & Computers
</a>
<a
href="#"
class="menu-link"
data-qa="hover"
>
Laptops & Computers
</a>

etojeDenys
etojeDenys suggested changes Nov 27, 2023
View reviewed changes
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.

/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@etojeDenys etojeDenys etojeDenys requested changes

@DarkMistyRoom DarkMistyRoom Awaiting requested review from DarkMistyRoom

@Volodymyr-Melnyk-Fron10der Volodymyr-Melnyk-Fron10der Awaiting requested review from Volodymyr-Melnyk-Fron10der

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Lazarend @etojeDenys @DarkMistyRoom @Volodymyr-Melnyk-Fron10der