Skip to content
OverviewSolutions
1
Comments
2
Matt Brannon
@ghostrib

All comments

  • Leo Silva•40
    @leozizzSubmitted about 2 years ago

    Hi, Frontend Mentor community! That's my solution for the Profile Card Component Challenge.

    Following the challenge, I tried to do the best I could within what I know and with a little research, but I believe I can improve the code in the future.

    I'm studying HTML and CSS with these challenges and would like some feedback to know what you guys think and what I can improve in my code.

    Any suggestions? :)

    Profile Card Component solution

    2
    Matt Brannon•70
    @ghostribPosted about 2 years ago

    Your solution looks really good. I have a couple thoughts.

    1. Your HTML starts with an h3 tag instead of an h1. This can cause accessibility issues for people who use screen readers. Generally, you need to have a single h1 tag and then h2 tags after that. Any h3 tags should go after h2 tags. You can style them however you like but the order of heading elements is pretty important for having semantically meaningful HTML.

    2. Your css looks really good. Just be aware that setting height to 100vh sometimes won't behave the way you want it to with mobile devices. Not an issue in this project, just something to be aware of.

    3. I think box-shadow is pretty standard now so I'm not sure if there's any need for -webkit-box-shadow. But it doesn't hurt to add it of course if you want to support older browsers.

    Nice work!

    Marked as helpful
    1
  • Basma•300
    @btebeSubmitted about 2 years ago

    Any feedback would be appreciated, thanks :)

    e-commerce-page

    1
    Matt Brannon•70
    @ghostribPosted about 2 years ago

    Hey nice work! I think it looks pretty good. A couple thoughts:

    When the mobile menu is open, tabbing through the elements closes the menu when tabbing away from the last item in the menu or tabbing backwards from the first item in the menu. Typically you want it to cycle back through the items in the menu. So it seems like your focus trap isn't quite trapping focus. I've used focus-trap-react myself and had varying degrees of success. As an alternative, you might look into @reach/dialog. The documentation is quite a bit better in my opinion and has a bunch of workable examples. Either way works though.

    Accessibility is difficult and I'm still learning a bunch about it myself. Heres's a link about accessibility in modals

    w3.org is good reference when dealing with these kinds of challenges.

    Hope that helps!

    Marked as helpful
    0