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

Develop #4870

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

Develop #4870

wants to merge 9 commits into from

Conversation

maristella12
Copy link

Copy link
Author

@maristella12 maristella12 left a comment

Choose a reason for hiding this comment

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

Copy link

@gitlherme gitlherme left a comment

Choose a reason for hiding this comment

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

Hello, @maristella12!

Great job 🎉

Just some points about the code. Can you please check?

Also, your style isn't being applied correctly as the reference.

Please check the comments and adjust your code to make sure that all is working correctly.

You can run npm run test before a new commit to check if is working correctly. Remember to check the reference and test and check if all tests pass.

src/style.css Outdated
}

/* Linha em branco antes da regra */
.html {

Choose a reason for hiding this comment

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

You are trying to stylize a class that doesn't exists. Maybe is just html instead of .html?

list-style-type: none;
display: flex;
align-items: end;

Choose a reason for hiding this comment

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

You can remove this blank line to keep the pattern on file.

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

Choose a reason for hiding this comment

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

Should be a nav instead of a div.

src/index.html Outdated
Comment on lines 33 to 54
<div class="logo"></div>
<div class="lista">
<ul>
<li>
<a href="#">
<img
src="./images/logo.png"
alt="imagem da logo"
/>
</a>
</li>
<li><a href="#">APPLE</a></li>
<li><a href="#">SAMSUNG</a></li>
<li><a href="#">SMARTPHONES</a></li>
<li data-qa="hover"><a href="#">LAPTOPS & COMPUTERS</a></li>
<li><a href="#">GADGETS</a></li>
<li><a href="#">TABLETS</a></li>
<li><a href="#">PHOTO</a></li>
<li><a href="#"></a></li>
</ul>
</div>
<div class="is-active"></div>

Choose a reason for hiding this comment

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

Needs to be wrapped by a header tag.

src/index.html Outdated
Comment on lines 33 to 54
<div class="logo"></div>
<div class="lista">
<ul>
<li>
<a href="#">
<img
src="./images/logo.png"
alt="imagem da logo"
/>
</a>
</li>
<li><a href="#">APPLE</a></li>
<li><a href="#">SAMSUNG</a></li>
<li><a href="#">SMARTPHONES</a></li>
<li data-qa="hover"><a href="#">LAPTOPS & COMPUTERS</a></li>
<li><a href="#">GADGETS</a></li>
<li><a href="#">TABLETS</a></li>
<li><a href="#">PHOTO</a></li>
<li><a href="#"></a></li>
</ul>
</div>
<div class="is-active"></div>

Choose a reason for hiding this comment

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

This div is not needed. You need to add the is-active class on a a tag on list. Like <a href="#" class="is-active">APPLE</a></li>

align-items: center;
}

.nav {

Choose a reason for hiding this comment

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

I think that is just nav instead of .nav here too.

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

Já fiz as alterações recomendadas.

@maristella12
Copy link
Author

Guilherme tá brabo ! Só consegui chegar até ai!

Copy link
Author

@maristella12 maristella12 left a comment

Choose a reason for hiding this comment

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

Còdigo corrigigo, aguardando revisão!

@maristella12 maristella12 requested a review from gitlherme April 30, 2024 11:20
Copy link

@gitlherme gitlherme left a comment

Choose a reason for hiding this comment

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

Hello, @maristella12!

Just a point about the readme. Please adjust into the comment too.

About your code, everything is fine now! Congratulations 🎉✨

Please remember to request a new review after adjust the README =)

readme.md Outdated
@@ -27,8 +27,8 @@ 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://tellabitt12.github.io/layout_moyo-header/)

Choose a reason for hiding this comment

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

Change to the correct Github User, link is not working

readme.md Outdated
- [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://tellabitt12.github.io/layout_moyo-header/)
- [TEST REPORT LINK](https://tellabitt12.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.

Change to the correct Github User, link is not working

@maristella12
Copy link
Author

Alterções do nome do usuário corrigidas.

@maristella12 maristella12 requested a review from gitlherme May 2, 2024 13:13
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