Order summary component

Community feedback

• 

  P

  Remus D. Buhaianu•3,125

  @MiculinoPosted almost 3 years ago

  Good job on completing your first challenge, Herson!

  I know it can be really intimidating at first, but with each challenge you complete, you become more confident in your coding skills. So, well done on completing this Frontend Mentor challenge!

  I had a look at your live solution and I also looked through your code and I have a few suggestions / observations I'd like to share with you.

  In no particular order, here they are:

  1. As @Drougnov pointed out, when you imported the font from Google Font, you only selected the 300 weight and that's why you can't use the bold weight in your design. Add the 600 or 700 weight to your import and it should work just fine after that.

  2. A best practice for CSS is to reset the margin and padding of all elements using the * selector. You can also add the border-box property which will make sure that the padding and margin are added to the element's total width and height. You can see an example here: https://stackoverflow.com/questions/46923610/css-resetting-margin-and-padding/46923672

  3. You might want to look into BEM methodology for a neat way to structure your HTML classes. If not, just try to keep in mind that your code should read like prose. Always use classes that can be understood by other developers, if they were to read through your code.

  4. Instead of using absolute values like px, try to implement relative units like rem. I saw you've used vh on the body element, but there was no reason to set a 100vh height on the body in that case.

  5. As @skyv26 pointed out, you don't have any media queries which means your project won't be responsive. You need media queries to make your project look good and structured no matter the device you use to see the website.

  All in all, you've done a great job for your first project. I also suggest you have a look through https://www.frontendmentor.io/resources and use those resources to improve your skills as a developer.

  Keep up the good work and looking forward to seeing more of your projects here on Frontend Mentor!

  Marked as helpful

  3

  ---

  

  Herson•220

  @HersonmeiPosted almost 3 years ago

  @Remus432

  Thank you very much for the tips!

  I've already used box-sizing: border-box in some tutorials, but I didn't really remember to put it in my code. Thanks for remembering, I'll try not to forget anymore!

  That's right, after taking some classes on absolute and relative values, I'm going to start applying the next projects.

  I added media queries to the project, I think I managed to implement it correctly. I applied for the 375px cell phone, but I was in doubt about how to apply it to the desktop (1440px) because my page was initially made based on the desktop.

  Do I need to do both media queries (375px and 1440px)?

  Thanks again for the tips, they were very useful and made me study some points so that the next projects come better.

  1
• 

  ROCKY BARUA•1,090

  @DrougnovPosted almost 3 years ago

  Hello, @Hersonmei Good job with the layout.

  I think you selected only 'font-weight=300' from the google font. I'm not sure though. You may check that out.

  Marked as helpful

  2

  ---

  

  Herson•220

  @HersonmeiPosted almost 3 years ago

  @Drougnov That's right! I thought I just needed to select the font and change the font-weight in the CSS that worked.

  Thank you for the tip!

  1

  

  ROCKY BARUA•1,090

  @DrougnovPosted almost 3 years ago

  @Hersonmei Glad to help :)

  0
• 

  Aakash Verma•6,710

  @skyv26Posted almost 3 years ago

  Hi! Herson, I just checked your work. You made it, it a really good initiate towards your progress.

  I noticed some issues.

  1. You Annual plan font size is too bigger than normal, either change your price h2 size or instead of h2 you can simply use p tag and adjust size with font-size property.

  2. You have not implemented media queries. Always keep in mind that more than half of the world try to open any website on their phone first. If your mobile response is not good then nobody will open that website on their laptop too.

  I hope you understand. Soon I will see your next solution.

  Good Luck

  Marked as helpful

  1

  ---

  

  Herson•220

  @HersonmeiPosted almost 3 years ago

  @skyv26

  That's right! The h2 tag was not adequate, I put the p tag and it looks very similar to the designer.

  The responsiveness part was really missing, I will work on that point now.

  Thank you very much for your tips!

  1
• 

  Leonard Appelgryn•320

  @KaptainCS3Posted almost 3 years ago

  Good job bro Concerning the bold issues you had with your code, also go through the sample style guide it help me alot. I go suck figuring out color just by look at the design, but when I discovered the style guide it became more easier.

  Marked as helpful

  0

  ---

  

  Herson•220

  @HersonmeiPosted almost 3 years ago

  @KaptainCS3 I agree, I was using the style guide but I really need to pay more attention to it.

  Thanks and good studies!

  0
• 

  Renato Gonçalves•30

  @renatoknotPosted almost 3 years ago

  @Hersonmei Great job! The only thing I noticed was missing was the button hovers. Detailed colors to use on Hover are in Design folder, file "active-states.jpg"

  0

  ---

  

  Herson•220

  @HersonmeiPosted almost 3 years ago

  @renatoknot Fala Renato! Colocar em português que fica mais fácil. =D

  Então cara, essa era dúvida minha pq como está no design, fala em 'active-state'. Com isso entendi que era para colocar no momento de clicar no botão, que vai ativar a pseudo-class :active. Mas vi todo mundo colocando como hover, acho que deve ser assim mesmo.

  0

  

  Renato Gonçalves•30

  @renatoknotPosted almost 3 years ago

  @Hersonmei Na verdade é hover mesmo, active serve para outras coisas, como mostrar a aba ativa por exemplo. Neste caso em um exemplo real, ao clicar no botao passaria para outra tela. Nesse caso entra o Hover.

  Marked as helpful

  0