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 -- Elizabeth, Divya, Rediet Fansite #26

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

Conversation

dnai55
Copy link

@dnai55 dnai55 commented May 19, 2023

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 Elizabeth, Divya, and Rediet! I left some comments in your PR. Nice job with your fan site! I looooved the Powerpuff Girls when I was a kid, so I really enjoyed your site. 🤩


<header>
<h1 class="page-title">The Ultimate Powerpuff Girls Fanpage</h1>
<h4>Your source for all things Powerpuff</h4>

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

Comment on lines +23 to +24
<main>
<section>

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 here since the <main> tag is already providing semantic organization for these images.

<img src="https://legendary-digital-network-assets.s3.amazonaws.com/wp-content/uploads/2021/05/12225331/Him_Powerpuff_Girls_1.png" alt="Him">
<img src="https://www.gamespot.com/a/uploads/scale_landscape/1603/16030002/3815177-mojo-jojo.jpg" alt="Mojo Jojo">
<img src="https://cdn.britannica.com/08/190808-050-CB26C47B/The-Powerpuff-Girls-Bubbles-Blossom-Buttercup.jpg" alt="The three Powerpuff Girls">
<img src="https://static.wikia.nocookie.net/powerlisting/images/4/45/SugarSpiceEverythingNice.jpg" alt="Powerpuff Girls origin story">

Choose a reason for hiding this comment

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

I'd recommend storing images locally in your assets folder. There's a couple of pros to rendering images this way

  1. Your site won't need to contact a 3rd party site to load the image, which can speed up page load times
  2. If the linked site moves the image or goes down, you may end up with broken links. So, having local images guarantees you'll always have access to the image.

</main>
<footer>
<h5><a href="https://www.imdb.com/title/tt0175058/">source</a></h5>
<h5>&copy; Elizabeth, Divya, Rediet</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! They're used to automatically construct a table of contents.

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

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