Skip to content
OverviewSolutions
20
Comments
10
Nenad Cosovic
@nenadmne

All comments

  • Nenad Cosovic•360
    @nenadmne

    Good solution.

    Check adding number and backspace in the middle of the card number input.

  • Nenad Cosovic•360
    @nenadmne

    Your solution is looking great with consuming everything what was asked from this challenge.

    Only thing i would suggest is wrapping classes with same attributes under 1 line, like this:

    body, .img-overlay, .price-section, .author {
     display:flex;
}

    Alse same could be done for justify-content: center; or align-items: center;

    Great solution, keep up =)

    Marked as helpful
  • Nenad Cosovic•360
    @nenadmne

    Your solution is very elegant and simple. I loved it =)

    Here are few suggestions:

    * {
    margin:0;
}

    Consider putting bellow margin a global parameter of font-size, and then on rest of the classes use rem as font-size matter. Also putting font-family: 'Outfit', sans-serif; would work great here if its only 1 family for whole page.

    .text h1 {
    font-weight: bold;
}

    Here instead bold you could have used font-weight: 700 (or 600); so it would look same as challenge heading :)

    Great solution by the way =)

    Marked as helpful
  • Nenad Cosovic•360
    @nenadmne

    Hello, i look into code

    <button>
  <div class="button_text"><img class="cart_icon" src="./images/icon-cart.svg" alt="cart">  <span>Add to Cart</span> </div>             
</button>

    From this html part, remove div that is inside button, and put button class class="button_text" instead. Div in not supposed to be inside button.

    After that go in css and type .button_text { text-align:center}. This will center img and text in button.

    To make space between them just margin it in css.

    Keep up =)

  • Nenad Cosovic•360
    @nenadmne

    Looks like a nice and simple solution.

    Only thing i would add is that icon position (eye in the center of equilibrium photo).

     <div class="icon-img">
 <img src="images/icon-view.svg" alt="icon-view">
 </div>

    Try to either use z-index or pseudo ::before to fix its background color, as it should be white on hover.

    Happy coding =)

    Marked as helpful
  • Nenad Cosovic•360
    @nenadmne

    Nice solution there. I spent some spare time to look inside your code.

    1.I think when switching from mobile to desktop version you should quit using % and give strict width and height. Its recomended for most sites to not get width over 800px so our eyes dont get tired moving around screen. My screen is so 1500+ so your solution was very much streched.

    2.Instead writting font-wight:700; or transition: 0.5s; under every class, you could wrap them all under 1 place like this button, .price span, .attribution { font-wight:700;}. When have same style for more then few classes, its always best to wrap them under 1 line so you change them easier and your code looks less stacked.

    Nice work and happy coding =)

    Marked as helpful
  • Nenad Cosovic•360
    @nenadmne

    I like your coding style, you kept it pretty simple. I noticed few things that you may consider in future coding.

    1.What would made it even simplier is that, if you have more classes with same attribute, you can wrap them all like this .sedans, .suv, .luxury { width: 202px; height:350px}. So instead repeating 3 times, you got their box dimensions on one spot. Easier to change them after, if need.

    2.Also this line @media screen and (min-width: 992px) { .main-container {display: flex;} is not needed becuz you already stated that display:flex; in mobile version.

    Nice solution, keep up =)

    Marked as helpful
  • Nenad Cosovic•360
    @nenadmne

    Container has overflow:hidden so left half of your box is hidden outside of container, because its position is relative to parent who has hidden overflow. Simple solution would be that parent for the box image is body, so its wraped out of overflow. body::after There is probably 20 more solutions arround just pointing out the most newbie one

  • Nenad Cosovic•360
    @nenadmne

    main { padding: 10px 10px 10px 10px; padding: 1%; }

    1.You are giving 2 padding attributes. If all paddigns are 10px, just write it down once.

    2.When you are doing same attribute to more classes like display:flex, for example, you could just do it like this: .post-block, .post-section, .introductory-section, main , body { display: flex } So instead writing it in 5 lines, in 5 different classes, you write it down in single line.

    Nice project, keep coding =)

    Marked as helpful
  • Nenad Cosovic•360
    @nenadmne

    Hello, i had some spare time so i look into your code and shere few thoughts.

    h1 { font-weight: bold; }

    // I would use just font-weight: 700; as you did in .category further bellow. //

    @media only screen and (max-width: 700px) { .prices { display: flex; align-items: center; }

    Here i saw you repeated some of the atributtes from previus code. When using @media you should write down only code that is changing further.

    Last thing i would suggest is that instead writting for example display:flex, for every class, you just tight them all under same line:

    .one, .two, .three, .four { display:flex; }

    nice project and happy coding =)