P
Micha Huhn• 220
@MichaHuhn
Posted
That looks really good, well done! Awesome that you learned much about SVGs. The tooltip/popover is also well made.
Here are two points that can be improved:
- Semantic HTML:
- As your class name
article-boxindicates, we created an article in this exercise. That's why we can use the semantic<article>element here instead of a<div>. - The main heading should always be the
<h1>element, even if there is only one heading on the page. The different headingsh1,h2,h3, ... are purely semantical. That means we shouldn't choose heading elements based on a desired style. The style can be adjusted with CSS. So in this exercise we should use the<h1>element, because it's the only heading on the page. Then we can style it accordingly like defined in the mockup.
- As your class name
- Naming:
- Just a small point: Currently, the function to toggle the "icons box" is called
showIconsBox(). Because its task is to toggle stuff, we could also name ittoggleIconsBox().
- Just a small point: Currently, the function to toggle the "icons box" is called
I hope that's a bit useful. Very good project.
Marked as helpful
0
Konrad• 370
@konradbaczyk
Posted
@MichaHuhn Thank you so for check my project :)
- I forgot about
<article>element, but now I will use this in my next projects. - about headings: I know rules which you described, but I've used
<h3>because I treated this article component as a fragment of a bigger project like landing page etc. - about naming: my mistake, you are right this function should be named as
toogleIconBox()
0
P
Micha Huhn• 220
@MichaHuhn
Posted
@konradbaczyk Your assumption about the headings is generally correct. Because there are no other headings on the page, the <h3> automatically becomes the <h1>, even though it's part of a smaller fragment.
1
