Amr Elsayed• 10
@AmrSayed74
Submitted
Please review my solution and give me feedback on it
ieeah
@ieeah
All comments
- ieeah• 40
@ieeah
Posted
Hi Sayed, first of all good job! Id' like to give you a few advices which would, in my opinion, improve your code :)
- You used a lot of different <article> inside of the main container, but single div, containing two different sections would have been a lott easier to style, and also more correct, since everything is part of the same element, and not different elements. For Example:
this would also make a lot easier the targeting of the elements in the css, I've seen you used a lot of :nth-of-type, which is really cool that you know, but used like this are a lot of energy and effort which could be used on a better way.
-
about the <article></article> tag itself, it would'nt be semantically correct: The articole tag is used for shop-articles, or blog-articles; of course you can use it as you prefer, but i think a regular div with appropriates classes would be better, but probably that's just my opinion :p
-
in the css, from line 119 to line 134, you use a lot of flex, and margins mixed together, but would had been a lot easier to a) not using <ul> and <li> but just using a regular <div> with "display: flex;" and "justify-content: space-between;" to reache the same result (maybe i'm missing some reasonss for which you decided for that ath, if it's the case, sorry :p)
Anyway, keep working and develop on!
Marked as helpful
1
