C22- Astry, Beenish, Somy, Tatyana V. #15
Conversation
Merge branch 'main' of https://github.com/beenishali693/group-fansite
…rid of two columns in gallery
…regardless of browser
…into gallery/harry-potter-gallery-page
| <title>Harry Potter Fansite</title> | ||
| <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=Cormorant+Garamond:ital,wght@1,300;1,400;1,500;1,600;1,700&family=EB+Garamond:ital,wght@0,400..800;1,400..800&display=swap" rel="stylesheet"> |
There was a problem hiding this comment.
I tend to prefer using the @import style of external font loading. A rule like the following can go in a style sheet, allowing the individual pages to not need to know anything about the fonts being used.
@import url('https://fonts.googleapis.com/css2?family=Cormorant+Garamond&display=swap');| <header> | ||
| <h1>Fun Facts</h1> | ||
| </header> | ||
| <nav> |
There was a problem hiding this comment.
index.html and characters.html place the nav within the header, while here and in gallery.html it's a sibling. I don't see a major reason for this structural difference (they appear mostly the same in each page), so try to keep the markup consistent, as this allows for more predictable styling.
| <li><a href="../index.html">Home</a></li> | ||
| <li><a href="gallery.html">Gallery</a></li> | ||
| <li><a href="#">Fun Facts</a></li> | ||
| <li><a href="characters.html">Characters</a></li> |
There was a problem hiding this comment.
Here and in index.html, all 4 page links are given (with the current page having a self link), while the other two pages omit their self link. I tend to like to keep the layout of the nav links consistent across all the pages, even showing the text for the current page, as it means the links don't "bounce" around in the header.
However, I also like to make the text for the current page not have an active link (so as not to appear clickable) which helps convey to the reader that this is where they currently are.
| </nav> | ||
| </header> | ||
| <main> | ||
| <section> |
There was a problem hiding this comment.
As this section is the lone child of main, it's not really adding any additional semantic content. I think this tag could be removed.
There was a problem hiding this comment.
One way to think about using sections is to put them in places where it would make sense to add a heading and that heading could be used to generate a table of contents. If the content doesn't rise to that level of organization, or made sense to have a heading, then section might not be called for. Currently, HTML validation reports an informational message that this section has no heading.
| <div class="image-container"> | ||
| <img src="../assets/images/Hermione_Granger.jpg" alt="An image of Hermione Grange"> | ||
| <p> | ||
| <span class="image-caption-1">Hermione Granger</span><br> | ||
| <span class="image-caption-2">The Wand-Waving Wonder</span> | ||
| </p> | ||
| </div> |
There was a problem hiding this comment.
article might be a good choice here over div. MDN recommends article for a variety of items, including product cards, and these character info cards feel very similar to that to me.
| .container > .book:last-child { | ||
| grid-column: span 2; /* Makes last book span both columns in middle */ | ||
| justify-self: center; | ||
| } |
There was a problem hiding this comment.
Since you know exactly how many books weere in your layout, selecting the final element works here. Note that if this were being dynamically generated (say from a database of books), we might not want to always set this style for the last element, but only when the last child is also an odd child, so we might also add the :nth-child(odd) pseudoclass.
| html { | ||
| background: url("../assets/images/Hogwarts.jpg") no-repeat center center fixed; | ||
| background-size: cover; | ||
| } |
There was a problem hiding this comment.
Nice styling to keep the background nicely positioned.
| width: 35px; | ||
| height: 35px; | ||
| content: ""; | ||
| background: url("../assets/images/Sorting-Hat-PNG.png"); |
| } | ||
|
|
||
| footer { | ||
| position: relative; |
There was a problem hiding this comment.
I'm not noticing an effect from the relative positioning here. In general, be careful using position as it can have unanticipated consequences on the overall layout of the page.
|
|
||
| footer { | ||
| position: relative; | ||
| width:100%; |
There was a problem hiding this comment.
Nit: ensure there's a space after : in the rule.
No description provided.