Skip to content
OverviewSolutions
8
Comments
10
notabhishekrai
@notabhishekrai

All comments

  • notabhishekrai•180
    @notabhishekrai

    Hey buddy. It seems that you have forgotten to push the image in github. Thats why there is no QR image only alternative text in your card. Check again.

    Also: you can center content with just body{ display: grid; min-height: 100vh; place-content: center; }

    Marked as helpful
  • notabhishekrai•180
    @notabhishekrai

    pretty good job

    however, the background image is messed up in large screen view and mobile view.

    few suggestion:

    1. use mobile first approach and work your way up
    2. your card is the <main> and your <body> is container in this case, so you don't need that extra div wrapper.
    3. use body{ display: grid; place-content: center; } that will center the card for you.
    4. use ::before and ::after in body to add those bubbles (pseudo classes), you can get rid of two more divs that way.
    5. its good practice to not give height to the elements but in this case it won't do harm so just keep this in mind.

    other than that good job. keep stylin !

    P.S you can use my solution as reference, its not perfect but i hope you can learn new things;

  • notabhishekrai•180
    @notabhishekrai

    wow you somehow seem to manage that with position but thats a lot of maths. use css grid, trust me its way way way better and cleaner.

    good job with the BEM, you can substitute double underscores with just - it will be more cleaner you don't have to follow BEM 100%, just the concept is enough, use - to separate block and element, trust me it will be more cleaner and readable

    few suggestions:

    1. in ::before and ::after, you can add image through content: ''; its done by content: url('path/image.png'); don't use background property in pseudo elements. and use left, right, top and bottom in percentage to position them absolutely as you like.
    2. i think there is a typo, i suppose you meant to write clip: rect(0,0,0,0); but you have written clip:rect(0,0,0,0,0); (one more 0) remember a rectangle has 4 side >_o

    other than those pretty good job. and good job on focusing on accessibility like using .sr-only.

    you can also use my solution as reference good day! 2)

    Marked as helpful
  • notabhishekrai•180
    @notabhishekrai

    pretty impressive. Few suggestions:

    1. use max-width on the container so that your cards don't stretch in bigger screens.
    2. "Reliable, efficient delivery" and Powered by Technology are two different headings. I see you have used span tag inside the h1, in my screen (1920px) they come in the same line so use h1 and h2 in respective order.

    other than that, pretty good. 1 like for hard work

  • notabhishekrai•180
    @notabhishekrai

    pretty good

    but you should use ::before and ::after in the body and then set the background color in the body. with before and after, you can position the images as you like. It should solve your responsive issue.

    I see you have used those svg as background.

    you can use my code as reference https://github.com/notabhishekrai/profile-card-component

    although it isn't perfect, i hope you can learn something from it, also you can give your feedbacks.

  • notabhishekrai•180
    @notabhishekrai

    I was experimenting attribute selector in css ([role="value"]). Hence, the mess.

  • notabhishekrai•180
    @notabhishekrai

    Great Job! But you should wrap all the code inside a <main> </main> (replacing div class = "container" with main class="container" should do) and use atleast 1 H1 tag. These two should solve your accessibility issues.

    for the hover effect, you can just use position relative to the parent and position absolute to child with top and left 0.

    max-width does help in responsive design. If you use max-width of 200px for an image, it will resize when you decrease the screen size but it wont resize(stays the same) when you increase the screen size (to whatever size). Unlike the normal width property.

    Marked as helpful
  • notabhishekrai•180
    @notabhishekrai

    Wow! This is crazy dude. I have tested the QR and it works. Although I don't do js, this is very impressive!

  • abbos•460
    @shomurodovabbos

    flex

    1
    notabhishekrai•180
    @notabhishekrai

    Impressive! Although I would recommend more padding left and right to the text and border radius of around 17-18 px on the card and around 10 to 12 px to the image. Looks like you have given a fixed height to the card Its not necessary to give height. But looks close enough. Hats off.

  • notabhishekrai•180
    @notabhishekrai

    Other than the box shadow, I don't see any flaws in the design. Great work!

    use box-shadow: 4px 2px 20px hsl(212, 45%, 89%);

    Marked as helpful