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

Angie - Sophia - Sel (Sapphire) #34

Open
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

selenetenorio27
Copy link

No description provided.

Copy link

@mmcknett-ada mmcknett-ada left a comment

Choose a reason for hiding this comment

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

Great job! The finished product looks very nice!

</header>

<section class="grid-container">
<div class="flex-container1">

Choose a reason for hiding this comment

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

You've done a great job using semantic HTML tags throughout this document! This div might be a good candidate for becoming an article tag.

Comment on lines +14 to +21
<header>
<nav class="nav-bar">
<ul>
<li><a href="index.html">Home</a></li>
<li><a href="facts.html">Fun Facts</a></li>
</ul>
</nav>
</header>

Choose a reason for hiding this comment

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

HTML doesn't expect anything to come between head and body. If you look at how Chrome interprets this, it will actually move this header inside the body. That's a good place for it to go! You could combine this with your <h1>GALLERY</h1>, too:

<body>
  <header>
    <nav class="nav-bar"> 
      <ul>
        <li><a href="index.html">Home</a></li>
        <li><a href="facts.html">Fun Facts</a></li>
      </ul>
    </nav>
    <h1> GALLERY </h1>
  </header>

You'd just need to update the styling of the header and h1 to make this work for you.

Comment on lines +20 to +27
<br>
<p>Planting and harvesting native ingredients such as corn and chile peppers in chinampas…
Manually grinding grain for tortillas on stone and mortar…
<br>
<br>
<br>
Traditional Mexican cuisine embraces an entire heritage of culture dedicated to maintaining Mexico’s indigenous customs and cultural traditions.</p>

Choose a reason for hiding this comment

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

You might try wrapping each paragraph in a p tag and setting the margin or gap between them to achieve the visual layout you want. This is a bit more expected in modern HTML than using br tags to get space between blocks of text.

Comment on lines +30 to +33
<br>
<br>
<p id="text">This content was created by:</p>

Choose a reason for hiding this comment

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

It's a good idea to indent things that are siblings at the same level. The way this is currently indented, I thought the br and p tags were nested inside of the top p tag at first.

Suggested change
<p>Mexican cuisine it's now internationally recognized as an Intangible Cultural Heritage of Traditional Cuisine!</p>
<br>
<br>
<p id="text">This content was created by:</p>
<p>Mexican cuisine it's now internationally recognized as an Intangible Cultural Heritage of Traditional Cuisine!</p>
<br>
<br>
<p id="text">This content was created by:</p>

Comment on lines +48 to +51
<h3>copyright 2023</h3>
</footer>
</body>

Choose a reason for hiding this comment

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

[nit] indentation. 😄

    <footer>
      <h3>copyright 2023</h3>
    </footer>
  </body>

Also, since the copyright isn't really heading a section of the site, it doesn't need to be in an h3.

<br>
<p id="text">This content was created by:</p>
<nav>

Choose a reason for hiding this comment

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

Are your names being used for navigation? If not, you could remove nav.

</nav>
</header>
<h1 class="shimmer">Mexican Cuisine: A Heritage of Tradition</h1>

Choose a reason for hiding this comment

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

Everything after the header looks like it's basically an article. You could use that!

Comment on lines +26 to +31
nav ul li a:hover {
color: #000;
background-color: #fff;
}

nav {

Choose a reason for hiding this comment

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

[nit] Try to get spacing consistent so that the rules line up visually. When you indent, your readers may think the indented line is intended to be nested! (You may encounter SASS in industry, where nesting rules in a CSS-like file is actually allowed.)

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