Skip to content
OverviewSolutions
20
Comments
10
Nenad Cosovic
@nenadmne

All comments

  • Nenad Cosovic•360
    @nenadmnePosted almost 2 years ago

    Good solution.

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

    0
  • Lada•30
    @ladaspcsnSubmitted almost 2 years ago

    I feel like this one was pretty easy to do, since it didn't require any adjustments for mobile version. The hardest part for me was making an overlay when hovering over the image, but I managed to do it by using absolute positioning and z-index.

    I would really appreciate any comments or advice on improving my solution.

    NFT preview card component

    2
    Nenad Cosovic•360
    @nenadmnePosted almost 2 years ago

    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
    1
  • Nenad Cosovic•360
    @nenadmnePosted almost 2 years ago

    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
    0
  • Nenad Cosovic•360
    @nenadmnePosted almost 2 years ago

    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 =)

    0
  • Nenad Cosovic•360
    @nenadmnePosted almost 2 years ago

    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
    0
  • Nenad Cosovic•360
    @nenadmnePosted almost 2 years ago

    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
    1
  • Nenad Cosovic•360
    @nenadmnePosted almost 2 years ago

    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
    0
  • Maame Yaa Serwaa Bona-Mensa•70
    @mbonamensaSubmitted almost 2 years ago

    I am happy I have been able to get this far. I struggled a lot with the background positioning of the patterns and then with the illustration woman and box. I decided to show what I have now and ask for help on how to properly do this.

    My issue with the illustration for the mobile was having it stick to the top of the container even with different screen sizes. I used percentages in the absolute positioning for this but it did not seem to work past around 300px screen sizes and below.

    The illustration box also does not stay in one place. I tried using position: fixed and it did not work as I wanted it to. I'll appreciate help on my positions for the different screen sizes. Also, feedback on accessibility is welcome.

    Responsive and Accessible Accordion Card using HTML, SASS and JS

    1
    Nenad Cosovic•360
    @nenadmnePosted almost 2 years ago

    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

    0
  • nelson•160
    @HyoganSubmitted almost 2 years ago

    -Just please take a time to look at my code and tell me what i've done wrong
    Thank in advance

    flex, flex , flex

    1
    Nenad Cosovic•360
    @nenadmnePosted almost 2 years ago

    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
    0
  • Nenad Cosovic•360
    @nenadmnePosted almost 2 years ago

    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 =)

    1