Cohort22 - Liubov Davidova, Maybellene Aung, Rong Jiang, Tatiana Snook #10
Conversation
There was a problem hiding this comment.
👍 good that you remembered to move index.html to the project root.
| <p> | ||
| Welcome to Plant Kingdom—a space to discover, care for, and appreciate the beauty and benefits of plants. Plants enrich our lives by purifying the air, enhancing our well-being, and bringing natural beauty into our homes and gardens. | ||
| </p> | ||
| <img src="assets/the_map_of_plants.jpg" alt="Trees" > |
There was a problem hiding this comment.
Consider writing a more descriptive alt text to make your site more accessible. Maybe something like alt="Map showing different plant species."
| </p> | ||
| <img src="assets/the_map_of_plants.jpg" alt="Trees" > | ||
|
|
||
| <h3>🌱 About Plants 🌱</h3> |
There was a problem hiding this comment.
I could also see lines 40-62 in a separate section tag. I don't think you need it here and it's not necessarily correct, but I thought I'd call it out to highlight how different people interpret what elements are needed on a page.
| <a class="menu-link" href="#">Facts</a> | ||
| </li> | ||
| <li> | ||
| <a class="menu-link" href="gallery.html" target="_blank">Gallery</a> |
There was a problem hiding this comment.
Do you mean for these nav links to open a new tab?
The other 3 pages for don't set the target attribute like target="_blank". Consider aligning how the navigation behaves across all four pages.
| <div id="summary"> | ||
| <p> This is a quick guide on how to take care of your plants!</p> | ||
| </div> |
There was a problem hiding this comment.
Prefer a more semantic element like section instead of div here since divs don't provide meaningful information about the structure of our pages.
The docs for the section element say that the element "represents a generic standalone section of a document, which doesn't have a more specific semantic element to represent it".
| <section> | ||
| <h2>🌱 Funny facts about plants and not only 🌱</h2> | ||
| <img src="../assets/Plants.jpg" width="400" alt="Plants"/> | ||
| <p> |
There was a problem hiding this comment.
I question whether the Paragraph element is the most appropriate element to use here because: "Paragraphs are usually represented in visual media as blocks of text separated from adjacent blocks by blank lines and/or first-line indentation".
See docs here.
Perhaps, it can be removed completely or replaced with a more semantic tag.
| background-size: cover; | ||
|
|
||
|
|
||
| } |
There was a problem hiding this comment.
| background-size: cover; | |
| } | |
| background-size: cover; | |
| } |
Remove unnecessary blank lines
| margin-left: 1em; | ||
| /* background-color: red; */ | ||
| max-width: 1000px; |
There was a problem hiding this comment.
| margin-left: 1em; | |
| /* background-color: red; */ | |
| max-width: 1000px; | |
| margin-left: 1em; | |
| max-width: 1000px; |
Remove commented out code
| flex-wrap: wrap; | ||
| /* align-items: flex-start; */ | ||
| justify-content: space-around; | ||
| gap: 20px; | ||
| /* background-color: red; */ | ||
| margin: 1em; |
There was a problem hiding this comment.
| flex-wrap: wrap; | |
| /* align-items: flex-start; */ | |
| justify-content: space-around; | |
| gap: 20px; | |
| /* background-color: red; */ | |
| margin: 1em; | |
| flex-wrap: wrap; | |
| justify-content: space-around; | |
| gap: 20px; | |
| margin: 1em; |
Like in our Python projects, commented code should be removed. Leaving commented out question might beg the question "why is this here? was it needed? should it be added back in?" For clarity and keeping your codebase clutter free, remove commented out code.
| @@ -0,0 +1,42 @@ | |||
| main{ | |||
There was a problem hiding this comment.
| main{ | |
| main { |
No description provided.