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

ready to push #5332

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

Conversation

MindControl01
Copy link

No description provided.

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.

Hi, there is no demo link, you need to add demo link to the PR description
image

src/index.html Outdated
</head>
<body>
<h1>Moyo header</h1>
<!--Header start-->

Choose a reason for hiding this comment

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

Suggested change
<!--Header start-->

src/index.html Outdated
Comment on lines 28 to 29
<div class="wrapper">
<div class="header__wrapper">

Choose a reason for hiding this comment

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

Redundant

Suggested change
<div class="wrapper">
<div class="header__wrapper">

src/index.html Outdated
</head>
<body>
<h1>Moyo header</h1>
<!--Header start-->
<header class="header__menu">

Choose a reason for hiding this comment

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

Suggested change
<header class="header__menu">
<header class="header">

src/index.html Outdated
Comment on lines 30 to 32
<a href="#!" class="header__logo-link">
<img src="./images/logo.png" alt="MOYO logo" class="header__logo-pic">
</a>

Choose a reason for hiding this comment

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

Fix code style everywhere

Suggested change
<a href="#!" class="header__logo-link">
<img src="./images/logo.png" alt="MOYO logo" class="header__logo-pic">
</a>
<a href="#" class="header__link">
<img
src="./images/logo.png"
alt="MOYO logo"
class="header__img"
>
</a>

src/index.html Outdated
Comment on lines 36 to 38
<li class="header__item">
<a href="#Apple" class="header__link js-scroll is-active">Apple</a>
</li>

Choose a reason for hiding this comment

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

Suggested change
<li class="header__item">
<a href="#Apple" class="header__link js-scroll is-active">Apple</a>
</li>
<li class="header__item">
<a href="#Apple" class="nav__link is-active">Apple</a>
</li>

src/index.html Outdated
Comment on lines 33 to 36

<nav class="header__nav">
<ul class="header__list">
<li class="header__item">

Choose a reason for hiding this comment

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

Fix classes everywhere

Suggested change
<nav class="header__nav">
<ul class="header__list">
<li class="header__item">
<nav class="nav">
<ul class="nav__list">
<li class="nav__item">

src/index.html 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.

please, provide the links from the readme.md file, or feel free to ask for some help in the chat

@MindControl01
Copy link
Author

MindControl01 commented Oct 27, 2024 via email

@MindControl01
Copy link
Author

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.

Hi, these links are wrong now, you need to fix it and update it
image
image

Also, your demo link isn't working, you need to make a deploy and check after the demo
image

readme.md Outdated
Comment on lines 45 to 46
- [DEMO LINK](https://https://github.com/MindControl01.github.io/layout_moyo-header/)
- [TEST REPORT LINK](https://https://github.com/MindControl01.github.io/layout_moyo-header/report/html_report/)

Choose a reason for hiding this comment

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

Suggested change
- [DEMO LINK](https://https://github.com/MindControl01.github.io/layout_moyo-header/)
- [TEST REPORT LINK](https://https://github.com/MindControl01.github.io/layout_moyo-header/report/html_report/)
- [DEMO LINK](https://MindControl01.github.io/layout_moyo-header/)
- [TEST REPORT LINK](https://MindControl01.github.io/layout_moyo-header/report/html_report/)

src/index.html Outdated
</head>
<body>
<h1>Moyo header</h1>
<header class="header">
<div class="wrapper">

Choose a reason for hiding this comment

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

Redundant wrapper

Suggested change
<div class="wrapper">

src/index.html Outdated
<li class="nav__item">
<a
href="#Samsung"
class="nav__link is-active"

Choose a reason for hiding this comment

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

This class should be only for first link, remove it everywhere

Suggested change
class="nav__link is-active"
class="nav__link"

Copy link
Author

Choose a reason for hiding this comment

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

I thought this function on the first element in figma show us how should work all buttons after hovering over them

src/style.css Outdated

/* Header start */

active-color {

Choose a reason for hiding this comment

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

Need to create variable with :root selector

Suggested change
active-color {
:root {

src/style.css Outdated
display: flex;
justify-content: center;
align-items: center;
position: fixed;

Choose a reason for hiding this comment

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

Redundant

Suggested change
position: fixed;

src/style.css Outdated
}

.nav {
width: 1200px;

Choose a reason for hiding this comment

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

Suggested change
width: 1200px;

src/style.css Outdated
align-items: flex-end;
color: black;
list-style-type: none;
padding-left: 420px;

Choose a reason for hiding this comment

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

Suggested change
padding-left: 420px;

src/style.css Outdated
}

[data-qa='hover'] {
color: #00acdc;

Choose a reason for hiding this comment

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

Use a variable for repeated color

@MindControl01
Copy link
Author

@MindControl01
Copy link
Author

MindControl01 commented Jan 4, 2025

Я не могу понять почему Test Report link не работает, уже всё перепробовал. Деплой сделал/

Another comments was successfuly debugged

Copy link

@anastasiiavorobiova anastasiiavorobiova left a comment

Choose a reason for hiding this comment

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

Try to fix repo settings and pick the gh-pages as the deploy branch
Or you can fork and clone the repo one more time to rerun the deploy script
Screenshot 2025-01-05 at 14 18 39

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