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 #4925

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

moyo_header #4925

wants to merge 3 commits into from

Conversation

gazebo88fire
Copy link

@gazebo88fire
Copy link
Author

dinaminahi
dinaminahi suggested changes May 3, 2024
View reviewed changes
Copy link

@dinaminahi dinaminahi 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 👍
Just few small fixes needed

readme.md Outdated
Comment on lines 30 to 31
- [DEMO LINK](https://<gazebo88fire>.github.io/layout_moyo-header/)
- [TEST REPORT LINK](https://<gazebo88fire>.github.io/layout_moyo-header/report/html_report/)
Copy link

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://<gazebo88fire>.github.io/layout_moyo-header/)
- [TEST REPORT LINK](https://<gazebo88fire>.github.io/layout_moyo-header/report/html_report/)
- [DEMO LINK](https://gazebo88fire.github.io/layout_moyo-header/)
- [TEST REPORT LINK](https://gazebo88fire.github.io/layout_moyo-header/report/html_report/)

src/style.css Outdated
}

.nav__item {
margin-right: 20 px;
Copy link

Choose a reason for hiding this comment

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

We don't need it and it is broken, please, remove
image

src/style.css Outdated
Comment on lines 74 to 78
/* .logo {
height: 40px;
width: 40px;
margin-left: 50px;
} */
Copy link

Choose a reason for hiding this comment

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

Please, remove commented code before commit, to keep the code clean

src/style.css Outdated
html {
font-family: Roboto, sans-serif;
font-weight: 500;
font-style: normal;
Copy link

Choose a reason for hiding this comment

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

Suggested change
font-style: normal;

It is default style

@gazebo88fire
Copy link
Author

I'm sorry. I've just made changes. Added, deployed. Can you see it?

@gazebo88fire gazebo88fire requested a review from dinaminahi May 8, 2024 06:54
roman-mirzoian
roman-mirzoian suggested changes May 8, 2024
View reviewed changes
Copy link

@roman-mirzoian roman-mirzoian left a comment

Choose a reason for hiding this comment

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

Nice job, but needs a bit of work.
Please pay attention to the comments.
And fix your link into description of pull request.
This links are not working.

image

image

src/index.html Outdated
Comment on lines 23 to 24
href="https://fonts.googleapis.com/css2?family=Roboto:ital,wght@0,100;0,300;0,400;0,500;0,700;0,900;1,100;1,300;1,400;1,500;1,700;1,900&display=swap"
rel="stylesheet"
Copy link

Choose a reason for hiding this comment

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

I think we don't need that many font sizes if you only use 3.

Suggested change
href="https://fonts.googleapis.com/css2?family=Roboto:ital,wght@0,100;0,300;0,400;0,500;0,700;0,900;1,100;1,300;1,400;1,500;1,700;1,900&display=swap"
rel="stylesheet"
href="https://fonts.googleapis.com/css2?family=Roboto:wght@400;500;700&display=swap"
rel="stylesheet"

src/index.html Outdated
</head>

Copy link

Choose a reason for hiding this comment

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

Suggested change

src/style.css Outdated
Comment on lines 5 to 7
* {
box-sizing: border-box;
}
Copy link

Choose a reason for hiding this comment

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

Don't use * selector (it impacts performance), better use html or body tag.

image

readme.md Outdated
@@ -27,15 +27,15 @@ The page should match the design Pixel Perfect: all the sizes, colors and distan

❗️ Replace `<your_account>` with your Github username and copy the links to `Pull Request` description:

- [DEMO LINK](https://<your_account>.github.io/layout_moyo-header/)
- [TEST REPORT LINK](https://<your_account>.github.io/layout_moyo-header/report/html_report/)
- [DEMO LINK](https://gazeb88fire.github.io/layout_moyo-header/)
Copy link

Choose a reason for hiding this comment

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

image

Decision:

Suggested change
- [DEMO LINK](https://gazeb88fire.github.io/layout_moyo-header/)
- [DEMO LINK](https://gazebo88fire.github.io/layout_moyo-header/)

Copy link
Author

Choose a reason for hiding this comment

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

roman-mirzoian
roman-mirzoian approved these changes May 8, 2024
View reviewed changes
Copy link

@roman-mirzoian roman-mirzoian left a comment

Choose a reason for hiding this comment

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

Well done!

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

@roman-mirzoian roman-mirzoian roman-mirzoian approved these changes

@dinaminahi dinaminahi Awaiting requested review from dinaminahi

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

Successfully merging this pull request may close these issues.
3 participants
@gazebo88fire @dinaminahi @roman-mirzoian