@MatejBumbera
Posted
Hi, first of all, congratulations on your solution! You did a great job in matching it to the design and I really like your code too. I just think you have some excessive code there.
-
The excessive code I mean is assigning classes to main, h1 and p tags. I think you could easily just style directly the main, h1 and p tags.
-
Another thing I noticed is that you used picture tag around img, and I'd say that's not necessary since the picture tag is used when you want to put there multiple images that change in response to screen width. You can look up more about the usage of picture tag here: https://www.w3schools.com/html/html_images_picture.asp
-
And one tiny thing I noticed is u used article tag, which is very good since it's semantic, but I wouldn't really use it here since article should be used on part of a webpage that should be independent, like u could just put it on a different webpage and it would make sense. I would replace it with section, which is also semantic. You can check the difference at: https://www.w3schools.com/html/html5_semantic_elements.asp
Overall, you did a great job and I wish you happy coding! I hope you find my review helpful :)
Marked as helpful
@T-Gomziakov
Posted
@MatejBumbera Hello! Thank you so much for your feedback, I greatly appreciate it!
I wrote the excessive code because I approached this project with a perspective of needing to use this component's classes somewhere else. However, I suppose I should save time both for myself and a person reading the code by not including those unnecessary classes.
@MatejBumbera
Posted
@T-Gomziakov That sounds like a good reasoning and if you prefer it that way, don't get discouraged by me. I just don't think it is necessary to create reusable styles if you aren't going to use them anyway :D
