out0fSpac3• 90
@out0fSpac3
Submitted
Hello again! Any comments are welcome!
Radek
@Radexman
All comments
- Radek• 490
@Radexman
Posted
Hey! Very nice job on that styling, it looks very similar to the design the only difference that I can spot is the bold font on button. I also looked through you code, there is no need to give alt descriptions for icons that don't give important context for overall accesibillity and the h4 headings should not be used without heading hierarchy.
Here is some further reading on the topic: https://www.contentkingapp.com/academy/headings/
Overall amazing job on getting your design so close to the original. Good luck on next projects and happy coding!
Marked as helpful
0 - Sithum Gimhara• 50
@sithum-gimhara
Submitted
Radek• 490@Radexman
Posted
Hi there Sithum!
Good job on that project! If you don’t mind, I will provide you with some feedback :)
So I checked your code and it appears that you used <h3> before <h1> tag and <h1> is repeated twice. This is considered a mistake because when using headings we want to stick to the so - called hierarchy. You probably did it because the font size was correct but you can just change it in css in no time. We typically want to start from h1 because it should be a main title on our page and move our way to h6. Here is some good resource on that topic:
https://www.contentkingapp.com/academy/headings/
Aside from that the code looks good, you have a good naming convention for classes. Very readable and self-explanatory.
Overall amazing work on that project! I hope I was a little bit helpful for you. Good luck on next projects and happy coding!
Marked as helpful
0 - Aziz Prabowo• 220
@azizp128
Submitted
Radek• 490@Radexman
Posted
Hi there mate! Good job on that project! If you don’t mind, I will provide you with some feedback :)
So I checked your code and it appears that you used <h3> and skipped the <h1> tag. This is considered a mistake because when using headings we want to stick to the so - called hierarchy. You probably did it because the font size was correct but you can just change it in css in no time. We typically want to start from h1 because it should be a main title on our page and move our way to h6. Here is some good resource on that topic:
https://www.contentkingapp.com/academy/headings/
Aside from that the code looks good, you have a good naming convention for classes. Very readable and self-explanatory.
Overall amazing work on that project! I hope I was a little bit helpful for you. Good luck on next projects and happy coding!
Marked as helpful
1 - Seth• 310
@AbdoX9
Submitted
Radek• 490@Radexman
Posted
Hi there, good job on that project! If you don’t mind, I will provide you with some feedback. So I checked your code and it appears that you used <h2> tag before <h1>. This is considered a mistake because when using headings we want to stick to the so - called hierarchy. You probably did it because the font size was correct but you can just change it in css in no time. We typically want to start from h1 because it should be a main title on our page and move our way to h6. Here is some good resource on that topic:
https://www.contentkingapp.com/academy/headings/
Aside from that the code looks good, you have good naming convention for classes, there is indentation and some good whitespace going on. Very readable and self-explanatory.
Overall great work on that project! I hope I was a little bit helpful for you. Keep going and happy coding!
0 - Ian Rioba• 450
@Rioba-Ian
Submitted
The desktop/mobile images were a bit challenging to change for the card but I wrapped it around a media query and set display: none for the desktop image.
Radek• 490@Radexman
Posted
Hi there, good job on that project! If you don’t mind, I will provide you with some feedback. So I checked your code and it appears that you used <h3> tag before <h1> and <h1> is repeated twice. This is considered a mistake because when using headings we want to stick to the so - called hierarchy. You probably did it because the font size was correct but you can just change it in css in no time. We typically want to start from h1 and move our way to h6. Here is some good resource on that topic:
https://www.contentkingapp.com/academy/headings/
Aside from that the code looks good, you have good naming convention for classes, there is indentation and some good whitespace going on. Very readable and self-explanatory.
Overall great work on that project! I hope I was a little bit helpful for you. Keep going and happy coding!
0 - Odo Peter• 130
@Odo-Peter
Submitted
Radek• 490@Radexman
Posted
Hi there, good job on that project! If you don’t mind, I will provide you with some feedback. So I checked your code and it appears that you used <h3> tag and no <h1>, this is considered a mistake because when using headings we want to stick to the so - called hierarchy. You probably did it because the font size was correct. No worries just add <h1> element and change its styling in css. We typically want to start from h1 and move our way to h6. Here is some good resource on that topic:
https://www.contentkingapp.com/academy/headings/
Also you just need to change the font sizes and font weights and your project will be as close to original as possible! But that's a really minor thing.
Overall great work on that project! I hope I was a little bit helpful for you. Keep going and happy coding!
Marked as helpful
0 - Ebubedike Edwin• 40
@Zubby126
Submitted
Radek• 490@Radexman
Posted
Hi there, good job on that project! If you don’t mind, I will provide you with some feedback. So I checked your code and it appears that you used <h2> tag and no <h1>, this is considered a mistake because when using headings we want to stick to the so - called hierarchy. You probably did it because the font size was correct for that text. No worries just add <h1> element and change its styling in css. We typically want to start from h1 and move our way to h6. Here is some good resource on that topic:
https://www.contentkingapp.com/academy/headings/
Also it appears that you added your styles inline. It is cool in small projects like this one but we want to separate our docs from each other eg. index.html, style.css, app.js all three as separate files connected via link and script tags in our html. It helps us keep our code readable and not make too much razmataz with other languages.
Overall great work on that project! I hope I was a little bit helpful for you. Keep going and happy coding!
0 - Edwin Jonathan• 90
@Elektrokat
Submitted
Good day, please can my code be reviewed, I didn't really have any issue with this, but could there be a better way to do this? Thank you in anticipation
Radek• 490@Radexman
Posted
Hi there! I checked your code and it is really good! The only thing I saw is a quite minor one, you named your classes “second”, “third” and “fourth” which is neat and short but it is not semantic so that we don’t know what “third” is. It is good practice to write short self-explanatory names for our id’s and classes so that other devs would understand our code better. Here is some good resource on that subject:
https://stackoverflow.com/questions/1790455/whats-the-best-way-to-name-ids-and-classes-in-css-and-html
I hope that was a little bit helpful for you! Good luck on next projects and happy coding!
Marked as helpful
0 - Aws• 20
@Sword133
Submitted
Radek• 490@Radexman
Posted
Hi there, good job on that project! If you don’t mind, I will provide you with some feedback. So I checked your code and it appears that you started with <h2>, this is considered a mistake because when using headings we want to stick to the so - called hierarchy. We typically want to start from h1 and move our way to h6. Here is some good resource on that topic:
https://www.contentkingapp.com/academy/headings/
Also instead of using <br> tag try to add some and font-size to your text, in this way it will wrap more naturally. Also adding padding-inline should help. Try to avoid using top, left to position elements on your page, it may cause some issues with responsiveness. If you are not aware of flexbox yet you should definitely check it out:
https://css-tricks.com/snippets/css/a-guide-to-flexbox/
It will help you a lot with positioning elements on the page. I hope I was a little bit helpful :)
Good luck on next projects and happy coding!
0 - Monica Munoz• 160
@monimunozalzate
Submitted
hi guys, i would love some feedback, any! everything is welcome, and thanks in advance Monica
Radek• 490@Radexman
Posted
Hi there, good job on that project! If you don’t mind, I will provide you with some feedback. So I checked your code and it appears that you have <h3> before <h1>, this is considered a mistake because when using headings we want to stick to the so - called hierarchy. We typically want to start from h1 and move our way to h6. Here is some good resource on that topic:
https://www.contentkingapp.com/academy/headings/
Also you named the classes “small” and “big” which is short and fine but we want to stick to semantic naming convention. That means that you can name your classes and id’s in a short self-explanatory way. For example “price-before” and “price-after”. Here is another awesome resource:
https://stackoverflow.com/questions/38019/whats-the-best-approach-to-naming-classes
Good luck on next project and happy coding!
0 - Jumagobe• 1,050
@Jumanjigobez
Submitted
Difficulty only on responsiveness and also the padding of the qr image but now am good and understood the use of flex-box. I am a beginner and at-least give me some feedback I will appreciate.This is my first trial.
Thank you frontendmentor.io
Radek• 490@Radexman
Posted
Hi there! I looked through your code and if you don’t mind I will provide some feedback :).
So first of all it is good to separate our styles and html, for small projects it is acceptable to write our css inside html but we usually write it in a separate file and then connect it via link tak inside the head element.
It appears that you don’t use <h1> element which is necessary to include even in small projects, we want to use heading hierarchy which starts with<h1> and moves to <h6>, it improves readability of our code and helps screen readers to understand content of our page better. Here is some good resource on that:
https://www.contentkingapp.com/academy/headings/
Hope it was helpful for you at least a little bit ;)
Good luck on next projects and happy coding!
Marked as helpful
1 - Atharva Hinge• 80
@36atharva
Submitted
Radek• 490@Radexman
Posted
Hey there! I’ve checked your code and if you don’t mind I will give you some feedback :)
First of all the class naming convention, we typically want to name our id’s and classes in a semantic way. Don’t get me wrong, “right” and “left” are fine but when other dev will be reading our code he might have a little headache, it is good practice to name our classes in short self-explanatory way eg. left - “img-box”, right - “product-description”. There is no need to add a class to our button if we have only one button on our page, just use the button selector.
In your code h2 appears before h1, when using headings we follow a so called hierarchy, we want to start from h1and move our way to h6 and if the styling is not how we imagined it, just change it in css. Here is some good resource on that:
https://www.contentkingapp.com/academy/headings/
Hope you find it somewhat useful, good luck on next projects!
Happy Coding!
Marked as helpful
0
