@Islandstone89
Posted
Hello, good job!
My suggestions:
HTML:
-
You could place the svg as an
<img>- an inline svg is only required when changing the colors. If the SVG has meaning, it needs an accessible name - addrole="img"and add atitleelement inside with a descriptive name. If the SVG is decorative, addaria-hidden="true", so screen readers can ignore it. -
Never have text in divs alone. "Learning" should be a
<p>, it doesn't need to be wrapped in a<div>either. -
I would use the
<time>element for the date:<p>Published <time datetime="2023-12-21">21 Dec 2023</time></p>. -
You don't need to wrap the elements in
.parent-divin divs. -
I would replace
idwithclass. -
Remember that screen readers recognize images by default, so you shouldn't include words like "image" or "photo" in the alt text. A suitable alt text for the profile image would be "Headshot of Gary Hooper".
CSS:
-
Paragraphs have a default
font-sizeof1rem, so there is no need to declare that explicitly. -
On "Learning", remove the
max-width. To restrain its width, adddisplay: inline-block. You can also remove the Flex properties - The top and bottom padding is equal, as is the left and right padding- hence, the text will get centered in the middle. I would add a bit more horizontal padding, though. If you changepadding: 6pxtopadding: 6px 10px, this will add6pxon the top and bottom, and10pxon the left and right.
Marked as helpful
@MukarramHaq
Posted
@Islandstone89 Thank you for yet another detailed review of my code.
I have a few questions, I tried googling them but, I couldn't find a straight answer:
- Is there a reason to not wrap the elements inside a
.parent-div? Or is it because of this particular scenario? - We can limit the width with
max-width? I know thatdisplay:inline-blockimplies no specific width so why should we use that?
@Islandstone89
Posted
@MukarramHaq
-
Divs are primarily used for grouping content. The profile image and text are both grouped in a div - there is no need to complicate the structure beyond that, especially since there are only 2 elements. If you had, let's say, 6 elements, and you wanted to separate element 1-3 from element 4-6, it would make sense to wrap each of those groups of elements in their own div.
-
We should let the content determine the width of the "Learning" element. You can use
max-width, but it is more used when you want to prevent lines of text from being too wide - using thechunit, which defines a max amount of characters before the text should wrap. In this case, we don't need to limit the text, as it is very short. As mentioned the padding along with the content, will give the element the width it needs.
Hope this makes sense :)
