Article preview component

Community feedback

• 

  P

  Micha Huhn•200

  @MichaHuhnPosted 3 months ago

  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:

  1. Semantic HTML:
     • As your class name article-box indicates, 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 headings h1, 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.
  2. 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 it toggleIconsBox().

  I hope that's a bit useful. Very good project.

  Marked as helpful

  0

  ---

  

  Konrad•370

  @konradbaczykPosted 3 months ago

  @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•200

  @MichaHuhnPosted 3 months ago

  @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