Skip to content
OverviewSolutions
13
Comments
11
Martin Kamír
@Martin-K-Kamir

All comments

  • Martin Kamír•490
    @Martin-K-KamirPosted over 2 years ago

    Hey, Brinda! Nice work on the card.

    Couple things i found to improve:

    1. I think it's not good using of having section tag for the img header. Also i wouldn't set border top left-right radius. I would just put overflow hidden to the container of the card so nothing is overflowing.

    2. You set to card-details justify-content and align-items these properties won't work since you didn't display flex or grid. I wouldn't use these properties in this situation. What i would do is display flex, flex-direction column and gap "some value". So each item in the container have same gap. In the design demo the title have bigger gap so i would set to the title margin-bottom to give it bigger space then other elements in that container.

    3. On each text element i found you declare a font-family. Just declare font-family on the body in css. Also you set everywhere padding-bottom to make spacing between elements i would use margin-bottom instead.

    4. On the button you have box-shadow in the design demo there is blurriness on that shadow. So you may forget declare third value for the box-shadow.

    Anyway you did nice work on the card i wish you very best and have happy coding

    0
  • Arturo Simon•1,785
    @artimysSubmitted over 2 years ago

    Hello,

    Another challenge done. Something I did learn is that hsl(value, value%, value%, 1) can take a 4th value for opacity. I usually converted hsl value to rgba.

    For the purple ish active color and box shadow, I took the bright-blue color and reduced opacity. But because bright-blue hsl values were duplicated 3 times in my code I wanted to dry(don't repeat yourself) it up.

    Wrote a sass function but didn't work when I applied it to a custom property. No errors. If anyone has any thoughts on it?

    @function brightBlue($opacity: null) {
  @if ($opacity) {
    @return hsl(245, 75%, 52%, $opacity);
  } @else {
    @return hsl(245, 75%, 52%);
  }
}
:root {
    --bright-blue: brightBlue();
}

    Happy coding!!

    Order Summary Component with Flexbox

    2
    Martin Kamír•490
    @Martin-K-KamirPosted over 2 years ago

    Hey @artimys,

    cool idea about the function. To make it work, you need to use interpolation.

    Add this to your code and it will work.

    @function brightBlue($opacity: null) { @if ($opacity) { @return hsl(245, 75%, 52%, $opacity); } @else { @return hsl(245, 75%, 52%); } }

    :root { --bright-blue: #{brightBlue(0.5)}; }

    Marked as helpful
    1
  • Martin Kamír•490
    @Martin-K-KamirPosted over 2 years ago

    Nice work @AditNovadianto!

    Few things to point out.

    1. The FAQ content disappears at viewport 820px and then comes back at. I could not find why in the devtools. I would need more time to look into it.

    2. When you get to 423px viewport the whole content goes off the screen. Not very friendly for mobile users. On that container you set width to 900px. Would be better if you changed it to max-width and added media query at max-width 500px changed the width value for something smaller like 300px.

    3. When you get to phone screen sizes the FAQ content is not very centered since you set margin-left: 100px. It is not good practice to center content with margins. Use flex instead.

    Anyway i think the solution is good just needs bit work. Wish you best luck in next projects :)

    0
  • Marcin Garski•350
    @margsoftbfSubmitted over 2 years ago

    Maybe someone can tell me what I could do better with my code, it is important for me to go in the right direction when i try do next challenges

    Order card finish project

    2
    Martin Kamír•490
    @Martin-K-KamirPosted over 2 years ago

    Hey, i just check your code little bit.

    Couple things to look after.

    1. On the box class you do not need to use grid. What i would do is just set it to flex column and center it.

    2. The logo do not appear. You added bad pathway for the img. Also all the properties are not necessarily mostly the absolute position. Next time just add width to 100% and it will fill up the whole space in that box.

    3. The mobile__bg and desktop__bg is not correct way how to add that background image. For example add to body the background color and then background image with url to that image.

    4. Remember the buttons do not inherit from the body font-family.

    Anyway if this is your first project i think it is pretty good job.

    Marked as helpful
    0
  • Martin Kamír•490
    @Martin-K-KamirPosted over 2 years ago

    Hey Nathalia, nice work! Everythings looks amazing and nicely done!

    Only what i would fix is the card that is not very centered for smaller screens.

    Also maybe for your next project try to use rem units not px.

    Marked as helpful
    1
  • Martin Kamír•490
    @Martin-K-KamirPosted over 2 years ago

    Hey nice work!

    The website looks good but not very finished. The spacing on texts are big on larger screens since you are using flex space-between or space-evenly. Use gap instead or margins. There is missing content. On smaller screen sizes the padding on main div's is big. Use media query and make the padding smaller. Also texts are not centerd on mobile screens. Also i would lower the header's hight mainly on phone screens.

    The website looks good just give it some work and it will be perfect.

    0
  • 👾 Ekaterine Mitagvaria 👾•7,600
    @catherineisonlineSubmitted over 2 years ago

    Hello, Frontend Mentor community! This is my solution to the Ping coming soon page.

    I appreciate all the feedback you left that helped me to improve this project. Due to the fact that I published this project very long ago, I am no longer updating it and changing its status to Public Archive on my Github.

    You are free to download or use the code for reference in your projects, but I no longer update it or accept any feedback.

    Thank you

    Ping coming soon page

    11
    Martin Kamír•490
    @Martin-K-KamirPosted over 2 years ago

    Nice work Catherine!

    Just few things i would point out.

    • The page is not very responsive on couple devices. The text is overflowing and the layout is weirdly positioned not very centered.
    • The social icons on the bottom are not very rounded. I would suggest on .social-media .fa-brands delete padding add flex center it out and width height 1.5rem, also the instagram icon is smaller then other icons so maybe scale it up little bit but that is small detail
    • Also i would fix the scrolling behavior on full size desktop since there is no more content below. The image makes the page scrollable.

    But still the website looks amazing and i love your work!

    0
  • 👾 Ekaterine Mitagvaria 👾•7,600
    @catherineisonlineSubmitted almost 3 years ago

    Hello, Frontend Mentor community! This is my solution to the Testimonials grid section.

    I have read all the feedback on this project and improved my code. Due to the fact that I published this project very long ago, I am no longer updating it and changing its status to Public Archive on my Github.

    You are free to download or use the code for reference in your projects, but I no longer update it or accept any feedback.

    Thank you

    Testimonials grid section

    33
    Martin Kamír•490
    @Martin-K-KamirPosted almost 3 years ago

    Wow, nice work! There is no much to say. The code looks good. Just wondering why set html font-size 13px?

    Maybe in next project try to use SASS/SCSS it's just the best thing to do css.

    Marked as helpful
    1
  • Martin Kamír•490
    @Martin-K-KamirPosted almost 3 years ago

    Nice work !

    Marked as helpful
    0
  • Katherin Meganis Phang•70
    @kmeganizSubmitted almost 3 years ago

    I finally done this challenge, I learn to have a thick skin to ask, glad there were constructive feedbacks given. I am proud of myself, although the result is not exactly the same as what expected.

    I have difficulty in doing the card container, how to collapse the row into a column when in mobile size in other words to be responsive.

    Also for the overlay. I think my way is not making the purple same as the sample.

    Open for feedback. Thank you!!

    Building a Design in Stats Preview Card Component Main

    3
    Martin Kamír•490
    @Martin-K-KamirPosted almost 3 years ago

    Like @grace-snow said. Doing bootstrap without not knowing css is very bad practice and won't get you far. In the end you still need to learn css. It's very bad pratice to learning any libaries before the actual language.

    Also if you are struggling and copying code from other people. You will not learn anything. At start you should do some courses on udemy or atleast on youtube so you can start write your code by yourself. Which is best practice.

    Marked as helpful
    1
  • 👾 Ekaterine Mitagvaria 👾•7,600
    @catherineisonlineSubmitted almost 3 years ago

    Hello, Frontend Mentor community! This is my solution to the QR code component.

    I have read all the feedback on this project and improved my code. Due to the fact that I published this project very long ago, I am no longer updating it and changing its status to Public Archive on my Github.

    You are still free to download or use the code for reference in your projects but I longer update it or accept any feedback.

    Thank you

    QR code component

    20
    Martin Kamír•490
    @Martin-K-KamirPosted almost 3 years ago

    Hi, i really like your solution. If i could suggest you few things from my experience, what i would tweak a little.

    • Since you are using rems. Set to html tag in css font-size: 62.5%. So 1 rem is 10px. As default 1 rem is 16px. So setting it this way will help you calculate how much is 1 rem. For example 32.1rem is 321px :D

    • As @ToHX said set body (or make some container class) min-height: 100vh and center it with flexbox. So you can simplify your code also like @BenjaDotMin commented.

    • For the box-sizing: border-box dont use it on body tag but like this -> html box-sizing: border-box; ... and ... *, *:before, *:after { box-sizing: inherit }. It is better explained here -> https://css-tricks.com/box-sizing/

    Marked as helpful
    1