Skip to content

alteração html e css#7481

Open
maykonfox wants to merge 7 commits intomate-academy:masterfrom
maykonfox:master
Open

alteração html e css#7481
maykonfox wants to merge 7 commits intomate-academy:masterfrom
maykonfox:master

Conversation

@maykonfox
Copy link

Checklist

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

❗️ Copy this Checklist to the Pull Request description after links, and put - [x] before each point after you checked it.

  • Header height is set in 1 place (for the links)
  • Content is vertically centered (for any header height)
  • CSS is used to show all letters in Uppercase (don't type them in HTML)
  • Logo is an image wrapped with a link
  • CSS Variable is used for a blue color
  • Pseudo-element is used for a blue line below the active link
  • Code follows all the Code Style Rules ❗️
  • The Google Fonts Configuration follows requirements.
    alt text

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.

Review Summary

Great work on the overall structure and for correctly using flexbox to build the header. Your use of semantic tags and CSS variables is also excellent. However, there are a few critical issues that need to be addressed. First, the browser's default margins have not been reset, which causes a white border around the page; please add margin: 0; to the body styles. Second, the CSS selector .logo img violates a core requirement [CHECKLIST ITEM #7] by using a tag name for styling. Please add a class directly to the <img> element and use that for styling instead. Lastly, the image path in index.html is absolute (/src/images/logo.png) and should be changed to a relative path (images/logo.png) to ensure it works correctly after deployment.


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

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

src/index.html Outdated
/>
<link rel="preconnect" href="https://fonts.googleapis.com">
<link rel="preconnect" href="https://fonts.gstatic.com" crossorigin>
<link href="https://fonts.googleapis.com/css2?family=Roboto:wght@500&display=swap" rel="stylesheet">

Choose a reason for hiding this comment

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

This line exceeds the 80-character limit specified in the code style guidelines. When an element has long attributes, it's best to place each attribute on a new line to maintain readability.

src/index.html Outdated
<header class="header">
<a href="#home" class="logo">
<img
src="/src/images/logo.png"

Choose a reason for hiding this comment

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

Using an absolute path like /src/images/logo.png for the image source can cause issues when the project is deployed to a subfolder (like on GitHub Pages). It's safer to use a relative path from this HTML file, for example: images/logo.png.

Comment on lines 5 to 8
body {
margin: 0;
font-family: 'Roboto', Arial, sans-serif;
background-color: rgb(201, 204, 204);
}

Choose a reason for hiding this comment

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

The task requires resetting browser default margins. The body element typically has a default margin which should be set to 0. Also, remember to reset default margin and padding for list elements like ul.

src/style.css Outdated
padding:0 40px;
background-color: #fff;
}
.logo img {

Choose a reason for hiding this comment

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

This violates checklist item #7, which includes the rule: "Don't use tag names for styling (except html and body)". Instead of using a descendant selector (.logo img), please add a class directly to the <img> tag in the HTML and style it with that class.

src/style.css Outdated
display: block;
width: 40px;
height: 40px;
src: url(images/logo.png);

Choose a reason for hiding this comment

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

src is not a valid CSS property and has no effect here. The image's source is defined by the src attribute in the HTML <img> tag. This line should be removed.

Copy link
Author

Choose a reason for hiding this comment

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

quando tiro a barra antes do src a logo desaparece

src/style.css Outdated
}
.nav_link:hover::after {
transform: translateX(-50%) scaleX(1);
color: var(--blue);

Choose a reason for hiding this comment

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

The color property has no effect on this pseudo-element because it doesn't contain any text (content: ''). The line's color is already being set by background-color. This property can be safely removed.

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.

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]

If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.

@maykonfox
Copy link
Author

alteraçao do src com criaçao da class"img".

logo

foram retirados o margin:0
o color do hover

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.

Review Summary

Excellent progress on this task! You've addressed most of the previous feedback very well. However, a couple of changes are needed before this can be approved. The automated build is failing due to a linting error in src/index.html; the crossorigin attribute on your <link> tag needs a value. You can fix this by changing it to crossorigin="". Additionally, to meet the pixel-perfect requirement, you need to reset the default browser padding on the <ul> element by adding padding: 0; to your .nav_list rule. Once these two issues are resolved, your solution will be ready!


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

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

src/index.html Outdated
<h1>Moyo header</h1>
<header class="header">
<a href="#" class="logo">
<img class="img"

Choose a reason for hiding this comment

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

Class names should be meaningful and not replicate tag names. A class like logo__image would be more descriptive and align better with the BEM-like naming convention you're using for navigation elements. This also relates to the checklist item 'Class names represent the meaning of the content (not the styles or tag names)'.

Copy link
Author

Choose a reason for hiding this comment

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

ok corrigido

src/index.html Outdated
<a href="#" class="logo">
<img class="img"
src="images/logo.png"
alt="logo"

Choose a reason for hiding this comment

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

The alt text should be more descriptive. For screen readers and SEO, something like "Moyo company logo" would be more informative.

Copy link
Author

Choose a reason for hiding this comment

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

ok corrigido

Comment on lines +23 to +25
.nav_list {
display: flex;
list-style: none;

Choose a reason for hiding this comment

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

The <ul> element has default browser padding that needs to be reset for correct alignment. Please add padding: 0; to this rule to match the Figma design. It's also good practice to add margin: 0; to reset any default margins on the list.

Copy link
Author

Choose a reason for hiding this comment

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

ok corrigido

src/style.css Outdated
color: var(--blue);
}
.nav_link.is-active {
position: relative;

Choose a reason for hiding this comment

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

This position: relative; declaration is redundant, as it's already defined for all links in the .nav_link class on line 37. You can remove this line for cleaner code.

Copy link
Author

Choose a reason for hiding this comment

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

ok corrigido

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.

Review Summary

I am approving your solution as an educational override to help you move forward. You've done an excellent job addressing the previous feedback on the crossorigin attribute and the <ul> padding!

The main area for improvement is matching the design "Pixel Perfect." The automated tests are failing due to small visual differences in spacing and alignment. To find these, download the report artifact from the failed build check—it will show you exactly what to adjust in your CSS. Also, remember to move the is-active class to the "Smartphones" link as specified in the design.

You're very close to a perfect submission. Keep up the fantastic work!


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

Footnotes

  1. Rate AI review example

Comment on lines +71 to +72
<a
href="#"

Choose a reason for hiding this comment

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

According to the Figma mockup, the "Smartphones" link should be the active one. To match the design perfectly, please move the is-active class and the aria-current="page" attribute from this link to the <a> tag for "Smartphones". The data-qa="hover" attribute should remain on this link as required.

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