Skip to content
OverviewSolutions
19
Comments
15

Furk

@doganfurkanTurkey600 points

I'm an engineer, but not software. I just like to create things, and frontend development is a perfect way to do it for me. Even though this is not my professional field right now, I'm very open to taking my chances to start working as a frontend developer.

I’m currently learning...

Next.js

Latest solutions

View more solutions

Latest comments

  • Furk•600
    @doganfurkan

    Hey, good job! My suggestions would be:

    • Use aspect-ratio for a perfect round shape
    • Try to avoid fixed widths and heights as much as you can. Especially for mobile, where the screen is already limited.
    • Use em and rem units instead of px
    • Your flex-item-1 classed div has a negative margin-bottom for what? Remove those margins and use "gap" for better control over margins.

    I think if you handle those, you're going to end up with an excellent job.

    Marked as helpful
  • Furk•600
    @doganfurkan

    Hey, congrats! This is a very good job. I want to help you with one thing, though: Your "Bookmark" button. Your icon there isn't aligned with the button.

    • For the bookmark button; remove the padding, and set justify-content to "space-between".
    • For ".divIcon", get rid of the absolute positioning.
    • For the icon image; set the margin to 0 and "vertical-align" to "bottom".
    • Set "align-items: center" for Btnbookmark div because it messes up the height of your bookmark button
    • And lastly, for bookmarkText div; set flex-grow to 1 and justify-content to center.

    I think these will solve the issue. And again, good job!

    Marked as helpful
  • Furk•600
    @doganfurkan

    Good work, but still need some fixes as anything isn't perfect:

    • I think we can start with an advice, instead of a blank card
    • Advice number is meant to be the advice id that is coming from the API, it is not that wrong with your approach but in the design it was id
    • Your fetch function has "cache". This is why it sometimes doesn't reconnect to the API. If you have a look at here you can learn more about it

    Congrats

    Marked as helpful
  • Furk•600
    @doganfurkan

    Wow, have you really "just" started learning web development? This is pretty darn good for a starter. Just a couple of things here:

    • You see, in the design the word "Perfume" is written in uppercase. For places like that, where you want to show text in uppercase no matter the HTML, you can use text-transform: uppercase in your CSS.
    • Your card has a width of 600px, and it doesn't shrink, at all. Because of that, I think you should set your breakpoint for the mobile view as 600px in your media query. Right now, someone with a screen width of 500px would suffer to view the whole card. You can just switch to mobile view under 600px.
    • I don't know if you noticed, but you have a little gap under the image in mobile view. That is the default behavior of the <img/> tag. You can add vertical-align: bottom to your CSS to get rid of that irritating gap.

    Apart from all that, it is mind-boggling that you just achieved this as a beginner. Huge Respect!

    Marked as helpful
  • Furk•600
    @doganfurkan

    Hello! Good job, congrats! Here are some quick fixes that you can do:

    • Your breakpoint for your media query is 1440px. It is a lot. I think you should bring it down to somewhere like 700px.
    • Your main problem with the width and height is due to your not assigning a max-width to your container. If you assign .container{max-width:1000px} you can restrict the maximum width of the design.
    • For mobile view, you are using align-items: center for your container. This results in each container having different widths. It is not the most pleasing thing to see. Just get rid of that and let them have 100% width of the container.
    • At last, your buttons can use some more margin-top for the desktop view.

    Again, good job! Keep coding!

    Marked as helpful
  • Furk•600
    @doganfurkan

    Hello there! You did a good job, congrats! I've some suggestions though:

    • Since you gave your body a height of 100vh, it sometimes doesn't show the whole content. If you change it to min-height, at least, you would give it room to scroll through content.
    • You have a div with the class of result-div, and you are using percentages to give it a size. Don't declare both the height and width. To have it as a perfect circle, I recommend giving it width: 50% (for example); aspect-ratio:1 styling. Aspect-ratio makes the height and width equal.
    • Your result-div doesn't show the elements in the center. If you make it a flexbox, assign the flex-direction as column, and apply justify-content: center you can have them centered. Also, get rid of that <br/> tag to adjust the gap.
    • Why do you limit your container to 375px for mobile? Your breakpoint is somewhat like 400px already. I think, do not leave that little space on the sides. Give it a width:100% for mobile and increase the padding.

    I think just those can make a big difference in appearance.

    Have fun building!

    Marked as helpful
View more comments