Skip to content
OverviewSolutions
2
Comments
3
Fanni K
@fananibanani

All comments

  • Fanni K•80
    @fananibananiPosted almost 2 years ago

    Hi there! Great job on this project!

    I'm quite a beginner myself, but one thing I can give you feedback on is the semantic HTML used in your project. As you can also see in the accessibility report, "Heading levels should only increase by one". You are not only missing <h4> heading, but the very crucial <h1> heading as well. Semantic elements in HTML are all the tags that clearly describe their content, such as the <header> tag you used. Using them will not only make your HTML look more organized, but it will also help people who are using assistive technology like screen readers or those who have slow internet and might initially view your website without the CSS loaded.

    As a best practice, <h1> heading should be used only once on one page, but it should definitely be used if headings are needed on the page (even if there are no headings, a short description in a <h1> heading that is visually hidden but that screen readers can pick up on might be a good idea). Don't skip heading levels, like you did with <h4>, it will mess up the logical order of the page. This may not seem like a big issue, but it's good to learn to not skip heading levels and the strange order can really mess with screen readers too. From the provided design, in my opinion only heading levels 1-3 are needed for this project.

    Another thing I noticed in your HTML is the usage of <section class="main">. In semantic HTML, the <main> element is available, so since you have a <header> and a <footer>, it would be best to use <main> instead of <section class="main">. As far as I know, using <section> is only recommended if there is no semantic HTML element that would describe the content better, so it is used more as a last resort. You could probably use <section> for the individual boxes in this case.

    I definitely don't have a very wide range of knowledge on this topic yet, but the following links might provide some initial info on this:

    https://developer.mozilla.org/en-US/docs/Glossary/Semantics

    https://www.freecodecamp.org/news/semantic-html5-elements/

    Hope this helps!

    Marked as helpful
    0
  • Brendan•1,120
    @OneManBannedSubmitted almost 2 years ago

    I was wondering if I should wrap the product images inside <button></button> tags since they are interactive elements activated by a user? I felt a little overwhelmed by this project in the beginning,. Although, I am quite happy with the end result.

    E-commerce-product-page-solution

    1
    Fanni K•80
    @fananibananiPosted almost 2 years ago

    Hi there! Your solution looks great!

    I'm not sure whether wrapping the product images inside <button></button> tags would work, I don't really have advice on this. I did go through the page with Narrator, and while it could find the alt text, I couldn't tab through the images at all. If you include alt text for product images, I'd say a more descriptive alt text would be better, but perhaps others think differently.

    I can't really say much about your code as I'm only starting to learn React now. But for accessibility, I see that the plus and minus buttons don't have any text that screen readers could pick up, so people using them have no idea what the buttons do. You could improve this in many ways, with an img alt text, a button aria-label or text inside the button that is visibly hidden. If you don't want screen readers to pick up on an image (for example an SVG), you can just add alt="" to it.

    Hope this helps, let me know if you have questions about any of the above!

    Marked as helpful
    0
  • Fanni K•80
    @fananibananiPosted almost 2 years ago

    Hi there! Congrats on your first solution, it looks great!

    I'm at quite a beginner level in frontend development myself, but I did notice one thing right away: when the screen is wider than 1440px the card is no longer centered. From what I can see, this is getting thrown out of place because your container class' width is relying on rem sizing, rather than a percentage of the screen's width.

    Hope this helps! :)

    Marked as helpful
    1