Skip to content
OverviewSolutions
13
Comments
7
Carlos E Alford
@webshuriken

All comments

  • Carlos E Alford•285
    @webshurikenPosted about 2 years ago

    Hi, Nice effort on the challenge! There are two things I wanted to feedback on:

    • A <button> and <a> should not be used together. They are similar but have different roles. Here is a little article about the differences: https://bitsofco.de/anchors-vs-buttons/
    • As the button doesn't have a descriptive text, think about using an aria-label so assistive technologies announce its purpose. (there are other ways to achieve this)

    Hope it helps and keep going :)

    Regards, Carlos

    Marked as helpful
    0
  • Axel•250
    @AxlMrtSubmitted about 2 years ago

    I've tried to be as perfect as possible on this one and i think, i've focused too much on it. I losted a lot of time on moving things around and i was kind of lost with the mobile to deskopt.

    For the js, it felt easy.. Still, I wasn't focus enough and i couldn't figure out how to do better.

    I've tried my best but it is still a way far from perfect. If someone has something to say to improve this challenge, i'll be glad !

    Waiting for comments, thanks for you interest !

    Article preview

    2
    Carlos E Alford•285
    @webshurikenPosted about 2 years ago

    Hi Axel, Like what you have done so far. I just have one more point of feedback.

    • The whole component should have been wrapped in a single tag so that you can reuse it if there are more cards to add. So let's say moving everything inside the <section> tag, this way all the related card component elements are housed together.

    Hope it makes sense. :)

    Marked as helpful
    1
  • Carlos E Alford•285
    @webshurikenPosted about 2 years ago

    Hi, the card looks great and is responsive. Read your README and thanks for the tip on using the filter property with an example!

    Only have two things to feedback on:

    1. The large screen one needs a little bit more padding to the right so the text lines up exactly.
    2. The <main> tags are not for creating cards, because you are only meant to have one pair and if you need to add more cards then it would fail the semantic test.

    Great effort loved the component.

    Marked as helpful
    1
  • Carlos E Alford•285
    @webshurikenPosted about 3 years ago

    This looks really good, soooo close to the design. Works well on all devices.

    Keep going! and happy coding

    1
  • Carlos E Alford•285
    @webshurikenPosted about 3 years ago

    I love that you took it a step further and did a React app. :)

    My feedback is more on the CSS and HTML side of the project.

    • You need to use Semantic HTML to make the page more accessible, this is the reason for the issues in your report. Make sure you use <section> or <header>. See HTML5 landmarks

    • CSS, looking at the live site I noticed the component is not responsive. Between 568px and 1070px the elements inside the card dont fit. It would be good to have a look at your media queries.

    Hope this helps somewhat :-). Have a good weekend

    Happy Coding

    Marked as helpful
    1
  • Carlos E Alford•285
    @webshurikenPosted over 3 years ago

    Nice job replicating the card. Apart from what Ahmed Faisal pointed out, I would recommend:

    • looking into HTML5 Semantics. It will help separate your content by its meaning and also improves accessibility.
    • also, not required, it would make reading code easier to read if you add indentation.

    Happy coding :-)

    0
  • Carlos E Alford•285
    @webshurikenPosted over 3 years ago

    I would suggest adding some information to the README file for people who dont know what the challenge is about. If I look at your github repo without knowing about this challenge, all i know is that is a challenge about a profile card component but I dont know what was required to complete it.

    I do like your end product.

    1