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

Ruby: Ariel, Carolyn, & Sam #24

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

Conversation

purplem00n
Copy link

No description provided.

Copy link

@kendallatada kendallatada left a comment

Choose a reason for hiding this comment

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

Hi Ariel, Carolyn, and Sam! I left some comments in your PR. Nice job with your fan site! I loved your site on the coffee. 😋 ☕ (Note: I'm choosing to ignore fact 2 because I love my morning cup of coffee lol)

Comment on lines +18 to +27
<!-- <nav>
<ul>
<li><a href="facts.html"><button class="btn" type="button">
Facts
</button></a></li>
<li><a href="gallery.html"><button class="btn" type="button">
Gallery
</button></a></li>
</ul>
</nav> -->

Choose a reason for hiding this comment

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

Be sure to remove unused code, print tests, and any comments that shouldn't be published before finalizing a project. This helps with the readability and maintainability of a project. 🧹😌

</nav> -->
<main>
<section class="center-img">
<h3>

Choose a reason for hiding this comment

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

Make sure you don't skip levels when using <h1>-<h6> tags for headings! Browsers use heading tags to construct a table of contents, so the different numbers on heading tags should be treated as different heading levels rather than as different style options. It's best practice to start headings at h1 and then move to h2 then h3 and so on as you need to nest sections. If all headers are at the top level, then all header tags should be h1. CSS should be used to resize and change the style as needed.

You can read more about heading tag best practices here: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Heading_Elements#usage_notes

</nav>
</section>
<section class="content">
<h5>

Choose a reason for hiding this comment

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

Make sure you're only using <h1>-<h6> tags for headings and not styling purposes! I'd recommend a <p> tag here.

You can read more about best practices here: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Heading_Elements#usage_notes

Comment on lines +28 to +29
<section>
<figure>

Choose a reason for hiding this comment

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

I'd recommend dropping the <section> tag since the figure tag already provides semantic organization. You should only use a tag like <section> if there isn't already another tag providing semantic meaning to a grouping of elements.

<main>
<div>

<section class="Container1">

Choose a reason for hiding this comment

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

Make sure you're being consistent with your class and id naming conventions. Best practice is lowercase with hyphens (-) separating words when needed (unless you're using the BEM convention).

grid-template-columns: 33% 33% 33% ;
}

.Container1 {

Choose a reason for hiding this comment

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

I'd recommend using more descriptive class names. Looking through your CSS files, there's no way of knowing what's being styled without looking at the html files, which will ultimately slow down development and make it harder to manage CSS rules as your files grow larger.

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.

3 participants