Skip to content
OverviewSolutions
7
Comments
9
Darshan
@its-subhash

All comments

  • Darshan•210
    @its-subhashPosted 9 months ago

    Hey @SONALI-NEGI your solution looks good but I have some suggestions to make for your Coding practice and Responsive design

    • Coding Practice:

    Everthing works beautifully but you can avoid writing same lines again, while using media query I saw that you are defining colors of card again but you don't have to do so because css will automatically take whatever color is defined previously.

    Also removing some unnecessary properties like in line 94 of style.css writing margin: auto auto can be reduce to writing margin:auto only.

    • Responsiveness:

    It is a good idea to not align cards vertically untill screen size is significantly narrow like screen width of 450px or 375px, that way it'll look much better, right now as you go smaller than 900px...cards just strech too much and looks good only when you reach mobile screen size.

    Lastly about border radius of this project, I saw you defining borders on so many places and then taking care of it while changing to other screen size...

    One better approach I could think of is to put all three card inside a single div, and defining border only once, it won't work in start(because I tried it myself) but once you define this divs overflow:hidden it'll work amezing and you don't have to care about it everywhere...

    you can refer my solution to get what am taking about, but believe me it's really WORTH TRYING.

    That's all...I hope you'll find these suggestions helpfull.

    Happy Coding

    0
  • Darshan•210
    @its-subhashPosted 9 months ago

    Your sure used bootstrap?? oooooooh dude you uploded wrong solution, check your solution once again.

    0
  • Darshan•210
    @its-subhashPosted 9 months ago

    @hp-8 Hey, your solution look good in first glance but it have some major problems specially regarding Responsiveness , so here are some suggestions which can improve it with least amout of efforts...

    • in .main{} theres no need to put height and width specially not something like vh or %, it just makes your code look terrible while checking it's responsiveness... the better way of handling width and height of continer is to let it be grow with elements you add inside of it. Or add margin and padding for inner space.

    so in your .main you can add padding:30px and remove height and width so it will grow with more link added in future.

    • for .press{} it's the same advice as .main but since your button should be bigger and of fixed with, you can use width:100% so it captures all the space available for button, also you can add some gap to look it better, i found gap:0.5rem better in your case.

    • for <button> you have set it's width to 15vw thats why content gets overflow while changing responisveness, so insted of that, now since you have enough space for buttons after changing .press{} properties, you can now set it's with to 100% and all button will be set.

    • in addition, buttons should change color while hovering so use button:hover{ background-color: hsl(75, 94%, 57%); }

    that would be better... and all set.

    Hope you liked my suggestions

    Happy Coding

    0
  • Darshan•210
    @its-subhashPosted 10 months ago

    @Mon-web-rgb Hey, I saw your codebase and it seems like you forgot to push media folders or asset folders... that's why picture isn't showing there...just push those folders on github and everything will be set.

    Marked as helpful
    0
  • Darshan•210
    @its-subhashPosted 10 months ago

    @RoksolanaVeres Hey, your solution looks amezing...

    it's just a minor thing which might slipped your mind is gap between letters of "ABOUT YOUR FURNITURE" it's more in design compared to your solution, and maybe it can be fixed when you apply same classes as on your "SHOP NOW".

    Otherwise it really looks good.

    Marked as helpful
    1
  • Darshan•210
    @its-subhashPosted 10 months ago

    Hey @kanyijr, your solution looks pretty good, although I have few suggestions to make.

    • Your solution looks like too broad in 1440px which is usual screen size, so to prevent that form happening you can decrease max-width:700px to max-width:345pxof .container{} so it'll look better in both devices.

    • According to requirement links should glow green while hovering but you changed the opacity, so insted of changing opacity you should change background color.

    • Card borders are missing too, so consider adding those.

    -Explore :root so you can store repetating values at a same place, it does looks insignificant on these small project but you have to use it on bigger projects so using it is a good practice.

    Hope you find these Helpful

    Happy Coding...

    Marked as helpful
    0
  • Darshan•210
    @its-subhashPosted 10 months ago

    Hey @paupalazzesi , your solution looks pretty good but I just came across two major problems i would like to address, and I have some advice to how to correct those with least amount of efforts.

    1.Fixed Footer : Footer or say attribution is fixed at one place so while scrolling of even normally it comes above card, this problem can be fixed by completely removing .attribution{} class and adding flex-direction:column and gap:2rem inside body selector.

    2.Scroll Bar: It's not that of a big deal and can be ignored but in my opinion there should not be need of a scroll bar for just a card. And the best solution for it is quite headache to do when you came this far, so the easiest way I can do it is you can put margin-top:16vh insted of margin:20vh in .card class, this will add margin to the top only and it'll be centered.

    Additionally I saw lots of margins and paddings on elements which isn't that good thing, so you can explore it's better alternative, like explore flex and grid little more.

    Rest of the code was really good, I hope you'll find this helpful.

    Marked as helpful
    0
  • Darshan•210
    @its-subhashPosted 10 months ago

    Hey @matiasroj0 it looks great, just a doubt...you intensionally made profile image shape like that?...like it's not completely circle.

    Marked as helpful
    0
  • Darshan•210
    @its-subhashPosted 10 months ago

    Hey @Temi-Michael , I saw your code and i have some suggestions.

    In HTML: 1.Removing Default styling from frontend mentor is a good idea.

    2.Creating own grid and using custom gap will give you more control over structure.

    3.I personally think that using anchor tag insted of button is a good idea, since you have to add links for those social media accounts.

    In CSS:

    1.In .profile-img, setting border radius to half of the length or width of image will work completely fine for making in circle, you can adjust few pixels if it won't work, but using 9999px is out of the world.

    2.You can put only one dimention of picture either it's width or it's height and other will get fit automatically without breaking it's ratio.

    3.It will be nice to put repetating things at one place like font-family, you can either make variables or put constant properties in body selector.

    4.I won't recommend using bold or other random font-weights, sticking with what's given in style guide is a good practice.

    ** Adding transition to button color was a nice idea.

    Responsive: Am not sure how it's working but those are some unexpected behaviour, it might be because of using Bootstrap, you should consider using at it once.

    Hope you liked my suggestions.

    Marked as helpful
    0