Skip to content
OverviewSolutions
9
Comments
4
ThanHtike（テンタイ）
@CGM-ThanHtike

All comments

  • Gabriel•170
    @ctrl-brokencodeSubmitted over 1 year ago

    Hello! This is my solution to this challenge. I really appreciate if you take your time to help me and give opinions on my code. I hope you're having a great day/afternoon/evening!

    Results Summary Component

    1
    ThanHtike（テンタイ）•110
    @CGM-ThanHtikePosted over 1 year ago

    Hello @Gabriel-H502 I reviewed ur code and here's my suggestions

    • It's a good practice to put your style.css at the lowest level in head tag bcoz u might have conflict in future like when u use bootstrap etc.
    • using fixed width is not good for responsive.
    • don't use !important like these, unless it's really necessary, it can cause unnecessary problems in large projects as far as I concerned. width: 200px !important; height: 200px !important; margin: 30px !important;
    • using rem on font-size is a better approach I think, I'm recommend to read this article for more info about rem and em. The Surprising Truth About Pixels and Accessibility
    • And for the design approach, you should make those spacing same as design file (ur card looks short bcoz of lacking space)
    • As you use display flex on your .results container, you can use gap property in it, (put like gap: 20px) and it looks much better.
    • Last but not least, instead of using margins to center ur card (as it's not good for responsive), use these instead
    • in ur body tag display: grid; place-content: center; min-height: 100vh;

    I hope these tips might help

    Marked as helpful
    1
  • France•100
    @LanXhanSubmitted over 1 year ago

    Day 7 As always feedbacks are welcome. I would like to know how to have an efficient way in switching the images from mobile to desktop and any advice for the code itself. Thank you

    Responsive base apparel coming soon page

    2
    ThanHtike（テンタイ）•110
    @CGM-ThanHtikePosted over 1 year ago

    The effiicient way of switching images is using source element

    I'll show u an example

    <picture>
<source media="(max-width: 639.9px)" srcset="images/image-product-mobile.jpg">
<source media="(min-width: 640px)" srcset="images/image-product-desktop.jpg">
<img src="images/image-product-desktop.jpg" alt="photo of Gabrielle Parfum bottle">
</picture>

    In this line of code, product mobile will be shown at the breakpoint lower than 640px. And the product desktop will be shown at the breakpoint higher than 640px.

    I hope this might help

    0
  • ThanHtike（テンタイ）•110
    @CGM-ThanHtikePosted over 1 year ago

    Everything looks good except one thing

    If u change the breakpoint lg:___ to sm:___ , it would be better. Because mobile view is showing at 1024px an it's not an ideal imo.

    Cheers

    1
  • HeroLeam•30
    @HeroLeamSubmitted over 1 year ago

    In my view it was a simple project to start my career, I had no difficulties, the project is visibly close to the challenge, I can't say if I did the development correctly and not to say the patterns in question of typing the codes.

    If you can evaluate and mention points that I can improve, thank you!

    Results summary component

    2
    ThanHtike（テンタイ）•110
    @CGM-ThanHtikePosted over 1 year ago

    Dear @heroleam, I had reviewed you code and found out some points that can be improved.

    • you style.css should be written at the lowest level in <head> tag because it might be overwrite by other other css classes, if u use bootstrap or other frameworks in the future.

    • form-circle-text-secundary, you should aware of your spellings (it might hurts sometimes)

    • Shouldn't use spam for everything, ex: <span class="form-summary-text">Summary</span> you can use h1 element here and <p> tag for paragraph.

    • You are declaring some unnecessary css properties like flex-direction: row; as the default flex-direction is row, u don't need to declare anything.

    I think the [youtube video] will help. (https://www.youtube.com/watch?v=rxnX1jdoI6c&t=65s)

    • You wrote all css again in style-mobile.css. I'm not sure about landscape and portrait, however, if u use like @media(max-width: 768px), all the css you are declaring are effected also for mobile screens and all you need to do is modify some styles for mobile in media query.

    Overall, You did a great job in making it almost same as the design. I'm also still learning to improve my skills and hope my review will help you to improve in some ways. Best Regards!

    0