Skip to content
OverviewSolutions
22
Comments
33
Nabil Wasir
@NabilWasir

All comments

  • Nabil Wasir•680
    @NabilWasir

    Great work as usual

  • Nabil Wasir•680
    @NabilWasir

    Here is how you can improve your solution:

    • Use <main></main> instead of div for the container

    • Your width and the card's height and width are way large in both the desktop and mobile versions. It doesn't even fit the mobile screen. Make the width and height smaller

    • You didn't implement the hover states, so implement them

    • The image at the bottom should have the same width and height. It should have enough border-radius to make it a circle and have a border of around 1px solid (the color of the design)

    • Use * { margin: 0 ; padding: 0 ; } to get rid of the default padding/margin that the browser put on the body

    And if you have any difficulty understanding the things I am saying, visit my solution to this challenge and have a look at the codes

    Hopefully, my feedback is helpful! :)

  • Nabil Wasir•680
    @NabilWasir

    Here is how you can improve:

    • Using <main></main> instead of <picture></picture>

    • Use the similar icons given in the design and wrap them in a <div></div> container and give the div border of the icon color to have the circle shown in the design around the icons.

    • You have also forgotten the hover states on the button and icons. You can apply the hover effect using the <svg></svg> given in the icon file itself, you just have to click the icon file to get the <svg></svg> of that icon. Then in CSS use: svg path { fill: white; (you can use the color of the icon if it's not given) }

    • Then you apply the hover state svg path: hover { fill: cyan; (Input the hover color) }

    • In the mobile version you haven't centered the social icons, you can use a container for them and make it display: flex; justify-content: center;

    • Use some margin-top on the heading of the mobile version.

    And If you have any difficulty understanding what I am saying, then just visit my solution to this challenge.

    Hopefully my feedback is helpful! :)

    Happy Coding

    Marked as helpful
  • Nabil Wasir•680
    @NabilWasir

    Great work

    Here is how you can improve your solution :

    • Use display: flex; justify-content: center; align-items: center; height: 100vh; ( Without it you can't center anything using align-items: center; )

    • You can use <main></main> instead of div/span to get rid of accessibility issues

    • Make the font-weight of the text to normal, they look slightly bolder

    • Use button:hover { background-color: transparent; border: 1px solid white( Use the color of the design ); transition: 400ms; color: white( Use the color of the design); }

    And if are having any issues understanding my feedback then watch my solution to this challenge to you clear your doubts

    Hopefully, you will find my feedback helpful.

  • Nabil Wasir•680
    @NabilWasir

    Great work, well done

    You can get rid of accessibility issues by replacing div/span of the main container with <main></main>

    Marked as helpful
  • Nabil Wasir•680
    @NabilWasir

    Great work on the desktop version

    You can use <main></main> instead of div/span to remove accessibility issues. And you haven't made the mobile version, If you want help you can visit my solution to this challenge.

    Hopefully, you will find my feedback helpful. If you do find my feedback helpful don't forget to mark this comment helpful!

    Marked as helpful
  • Nabil Wasir•680
    @NabilWasir

    Great work

    You forgot to put the hover/active state on the question.

  • Nabil Wasir•680
    @NabilWasir

    Everything is way bigger on the desktop, it's not responsive and there is no mobile version. And you also got a lot of HTML validation errors, read them and try to fix them.

    If you need any help watch my solution to this challenge.

    Hopefully, you will find my feedback helpful. If you do find my feedback helpful don't forget to mark this comment helpful!

  • Nabil Wasir•680
    @NabilWasir

    You have got a few problems with your solution

    Here is how you can fix them :

    • Keep your image files in the same folder as your HTML and CSS file and the <img src=""</img> set the src="" correctly or else the images won't show up

    • You don't have enough padding/margin on the text, you should apply some margin/padding to them

    • Your font sizes aren't correct, especially since the names of the people are way large. Make the names smaller

    And if are having any issues understanding my feedback then watch my solution to this challenge to you clear your doubts

    Hopefully, you will find my feedback helpful. If you do find my feedback helpful don't forget to mark this comment helpful!

    Marked as helpful
  • Nabil Wasir•680
    @NabilWasir

    Great work, the site looks great.

    But I think you should use the cursor: pointer on hovering the social icons. And to get rid of accessibility issues use <main> </main> instead of div/span of the main container.

    Hopefully, you will find my feedback helpful. If you do find my feedback helpful don't forget to mark this comment helpful!

  • Nabil Wasir•680
    @NabilWasir

    Great work

    But I think the cards are slightly bigger on all devices. Maybe making it slightly smaller will make it look like the actual design.

    Here is how you can get rid of accessibility issues:

    • Use <main></main> instead of div/span etc.

    • Use headings( h1, h2, h3 etc. ) serially. Use h1 then h2, then h3, don't just use h3 instead of using h1 in the begining

    Hopefully, you will find my feedback helpful. If you do find my feedback helpful don't forget to mark this comment helpful!

    Marked as helpful
  • Nabil Wasir•680
    @NabilWasir

    You have made a few mistakes, I would suggest you use dev tools on your browser or see my codes of this challenge to fix it.

    Here is how you can get rid of accessibility :

    • You should use <main></main> instead of div/span

    • You can use <footer></footer> instead of <div></div> in the attribution section

    Hopefully, you will find my feedback helpful. If you do find my feedback helpful don't forget to mark this comment helpful!

  • Nabil Wasir•680
    @NabilWasir

    Great work

    Here is some suggestion to make your website look like the actual design :

    • You forgot to implement the active states of the buttons and social icons. You should also use cursor:pointer on buttons and social icons.

    • There is not enough padding/margin on the footer links, add 10px or more padding-top/margin-top on the links to have a better visual hierarchy

    Hopefully, you will find my feedback helpful. If you do find my feedback helpful don't forget to mark this comment helpful!

    Marked as helpful
  • Nabil Wasir•680
    @NabilWasir

    Great work

    Almost everything looks great, but I think everything is a little bit larger in the desktop version. I think if you make them slightly smaller it will look perfect. And you also got a lot of accessibility issues and HTML validation, please try to fix them by reading the error.

    Hopefully, you will find my feedback helpful. If you do find my feedback helpful don't forget to mark this comment helpful!

  • Nabil Wasir•680
    @NabilWasir

    Great work

    • I see you are having an issue with the overlay on image hover. You can fix it by using a div and positioning it absolute on the card. You can see my solution to this challenge to understand what I am saying.

    • You should make the cursor: pointer on the hover state

    • You can use a <footer> </footer> instead of a <div> </div> in the attribution section and have an alt attribute and keep it empty or fill it with the text that you want to fix the accessibility issues

    Hopefully, you will find my feedback helpful. If you do find my feedback helpful don't forget to mark this comment helpful!

    Marked as helpful
  • Nabil Wasir•680
    @NabilWasir

    Hello there, great work on the desktop version

    But I think you haven't made the mobile version and the website isn't responsive. You can see my solution to this challenge to improve it. Hopefully, it will help you.

    And you should use semantic HTML Tags like <main></main>, <section></section> etc., If you use these semantic HTML tags then you won't get an accessibility issue.

    Hopefully my feedback is helpful to you

    Marked as helpful
  • Nabil Wasir•680
    @NabilWasir

    Great work 👏

    I think the width of the card on the desktop is way large than it should be, you can decrease it to make it look like the actual design. Everything else looks fine.

    Hopefully, you find my feedback helpful. And if you don't forget to mark it as helpful

    Marked as helpful
  • Nabil Wasir•680
    @NabilWasir

    Hello there, great work 👏

    You haven't used enough line height on the heading and sub-heading. I would recommend you use more line-height to make it look better like the actual design. You can also use padding-right on the heading to make it look like the actual design. But everything else looks great.

    Hopefully, you find my feedback helpful. And if you don't forget to mark it as helpful

    Marked as helpful