@RichardOgujawa
Posted
Hi Arsenije,
Hope you're keeping well:) I really like your coding solution, especially the fact that you indented your code. It makes it easy to read. I appreciate how you used comments to write headings in your CSS to make it more easily navigable.
However, I would make some changes to the code. Then again these are just my opinion so feel free to take this on board or take it with a grain of salt, I wish you all the best either way.
In the HTML:
- I'd recommend wrapping the main content of your project with a main tag, even if it's the only element on the page.
- Check out BEM Naming convention to make naming classes easier and also to make it easier for other developers to understand your code better.
- Heading levels are used for the most to least important text on the page, since that's the case I'd recommend having the 'perfume' text as a h2 because it can be considered as a category tag, and leave the price as spans or paragraph text.
- I'd recommend using an article tag for the product card as a whole, because it would be regarded as a piece of content that can be understood without any information around it, which is all an article really is in HTML (https://developer.mozilla.org/en-US/docs/Web/HTML/Element/article)
- It isn't the best idea to use the <s> tag to put a line through your text because it results in that bit of text not getting read out by screen reader software. More on that here: https://www.tempertemper.net/blog/be-careful-with-strikethrough Instead, you could just wrap that bit of text in a span and do something in the CSS like text-decoration: line-through (https://www.w3schools.com/cssref/pr_text_text-decoration.php)
- I wouldn't recommend using class="heading" for a heading tag. If you want to describe the heading tag better I'd recommend something like class="title" or class="subtitle", etc.
- When it comes to HTML you want to make it as meaningful and descriptive as possible. If something is a button you want to use a button tag, if something is a paragraph use a p tag, etc.
- I'd recommend using a picture instead of just the image tag because it's a much better way to add responsive images to your websites instead of just relying on CSS. CSS simply scales up and scales down an image, so if you use only one image to cater for the fact that it would be on both larger screens and smaller ones, you'd have to use a high-resolution image in your src for the image tag. However, not all users need that high-resolution image, and would prefer to load a lower-resolution one because their device might be slow as is, or they may just be using a device with a smaller screen and therefore loading a high-resolution file would be overkill, because even though it's scaled down it's still a high-resolution file meaning it's a large file to load. More on all that here: (https://blog.bitsrc.io/why-you-should-use-picture-tag-instead-of-img-tag-b9841e86bf8b).
- Also when it comes to heading levels, they should only ever decrease by one at a time, so you shouldn't ever really have a page with say a h1, h3 and h4. If you only have three headings levels on a page, it should go down by one level at a time, i.e. h1, h2, h3. Because at the end of the day heading levels are used for ranking headings from the most to least important on the page.
- Icons and image (unless it's purely decorative) should have an alt text attached to it. The only other time you may not want an alt text on your icons is when it's beside text that essentially describes what it is, such as an icon of a happy dog beside beside text that says 'happy dog' because if you include the alt text the screen reader would end up reading out "happy dog, happy dog."
In the CSS:
- An easier way to center content in a container would be to use {display: grid; place-content: center;}
Hope this helps:) And if you want me to explain anything a bit better just let me know, I was trying to keep this not too long, so I had to rush through some concepts.
Marked as helpful
@ArsenijeDragic
Posted
@RichardOgujawa Hi!
Appreciate your feedback! I went over it briefly as I'm preparing to finish my day and the concepts you mentioned seem pretty useful. I will definitely dive deeper tomorrow as I'm always eager to learn new things.
Constructive feedback is always welcome. Thank you very much!
@RichardOgujawa
Posted
@ArsenijeDragic Happy to hear that and happy to help. And as always feel free to let me know how it goes, and if you want to go over anything.
All the best!
@ArsenijeDragic
Posted
@RichardOgujawa Hi, hope you're doing well.
I updated my solution using the methods you told me about earlier. When you have time, can you please check it out and let me know your thoughts? Please do keep in mind that these things are still brand new to me so I might've got some of them wrong (feel free to point out).
In addition, i noticed that i wasn't able to apply "border-radius" to my article tag for some reason.
Also, please ignore that the dimensions are a bit off.
Appreciate your help!
@RichardOgujawa
Posted
@ArsenijeDragic I'm all good bud, hope you're keeping well too!
Yeah, would be more than happy to, I'll check it out now and will share my thoughts after.
I'll take a look at the border-radius one first since it's something you pointed out. And don't worry about not getting it perfect, as you specified you're new to it, these things will come with time. All you need to be focused on is being better than you were yesterday.
And the dimensions for mine aren't perfect either so it's okay man don't worry 😂, these challenges aren't really about getting things pixel perfect, they're more about being able to get the structure right from a visual stand-point and also improving your code with the help of tips, comments and remarks from the FrontEnd Mentor community. I'd say you're doing great so far👌🏾