Skip to content
OverviewSolutions
3
Comments
3
Jacob
@theschmocker

All comments

  • Jacob•140
    @theschmockerPosted over 2 years ago

    Really great work on this! Solid, semantic HTML and the styles are really close to the design.

    If I may, here are some suggestions for accessibility improvements.

    One major concern is the toggler. Currently it is not keyboard or screen reader accessible. One possible solution to this would be turn it into a visually hidden checkbox with a label like "Show Annual Pricing" and some focus styles. That'll make it usable for keyboard users and will help screen reader users understand what it does. Here's a helpful article from CSS-Tricks with a code snippet for visually hidden styles that could be useful for a checkbox and its label: https://css-tricks.com/inclusively-hidden/

    Some more minor tweaks to help make things easier to understand with a screen reader:

    • Add a visually hidden h3 "Features" above the each of the features lists. That will help add context to what the lists represent
    • Using VoiceOver, the prices currently read like "dollar" (move to next element) "nineteen point ninety nine". Something I've done in the past to help with this kind of stylized pricing element: apply aria-hidden to an element that wraps the dollar sign and the other pricing information. Outside of that element, add a visually hidden element that contains the price as it would normally be written, eg. "$19.99". I've found that VoiceOver will read it correctly as "nineteen dollars and ninety nine cents. You might be able to play with it a bit and simplify what I've suggested.
    • Add some visually hidden text to the "Learn More" links. Something along the lines of 
      <a href="#" class="learn-more">Learn More<span class="sr-only"> about the basic plan</span></a>
      Screen-reader users sometimes navigate a page by its structure and/or the links and buttons it contains. With many links reading the same, it's hard to determine what "Learn More" is referring to. That little bit of extra context can make the page easier to navigate!

    Hope this helps!

    1
  • Shruti Tewaney•30
    @shruti072003Submitted over 2 years ago

    Hello, I've tried to not commit the accesibility errors that my last challenge solution had. It would be nice if you could give me any feedbacks or suggestions regarding this solution.

    Responsive landing page

    1
    Jacob•140
    @theschmockerPosted over 2 years ago

    Always happy to see folks focusing on accessibility 🙂

    Some notes for accessibility improvements:

    • A page should have a single h1 element. In this case, I think "Order Summary" (which is currently an h2), would be good as an h1.
    • Heading levels should not be skipped. Right now, the next heading level after h2 is h5. With the change to h1 above, I recommend changing the h5 to an h2. Some helpful notes here/Heading_Elements#accessibility_concerns
    • Only images need alt attributes; they should be removed from the div elements
    • Images only need alt text when they're important for understanding the content. In this case, I think all of the images are decorative. Decorative images do still need an alt attribute, but it can be left empty, ie. alt="". This signals to assistive technology that the image can be ignored. More information here

    In case you haven't heard about them: check out axe DevTools and WAVE. They're not perfect--can't catch everything and sometimes throw false positives (WAVE is worse about this, in my experience)--but they're really helpful for catching accessibility errors we all miss sometimes

    Style-wise, your solution looks great! If I'm being picky, here are a few things to maybe consider to get even closer to the design:

    • Set the width of the hero image to 100%; should help center it
    • Tinker with box shadow on the button and see what you can come up with... box-shadows can be tough to eyeball
    • Tweak padding, line-height, and font-size on various elements. These can be tough to eyeball without a Figma/Sketch/XD mockup too

    Good work, hope this helps!

    Marked as helpful
    1
  • Jade•105
    @jadetrueSubmitted over 2 years ago

    I'd love general feedback on this project.

    The main area I'd love feedback on is how I've displayed the Cards. I've accessed the data individually rather than mapping through my array of objects - mainly because the styling is varied per card and with the layout, it made sense this way.

    I am attempting to learn different sections of React at a time and I started off with context to be able to keep all my data in one file and pass it down to the components that need this information.

    Testimonial Grid (CRA & Tailwind CSS)

    1
    Jacob•140
    @theschmockerPosted over 2 years ago

    Overall, looks great!

    Design-related thoughts:

    • There are a couple of profile images that aren't quite circles because the images themselves are not square. Something you may consider trying is to set an equal width and height on the image and applying "object-fit: cover". That will crop it to fit within the desired dimensions without making it squish.
    • Consider dropping into the single column layout a little sooner at intermediate sizes, some of the skinnier columns get pretty small. Or try experimenting with alternative multi-column layouts at those sizes that have a similar vibe as the large layout, but with less columns
    • Adding some padding to the container will help keep the content from hugging the edges of the screen on smaller mobile sizes
    • Setting a max width on the grid will help match the design more closely

    Semantics/Accessibility

    • A page should have a single h1. https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Heading_Elements#multiple_h1_elements_on_one_page What I often do when there's not a clear heading for a page or section is to add one and hide it visually using an "sr-only" class, or similar. That helps to provide context to screenreader users that may be lost when relying on visual hierarchy alone
    • I think that have both sections of the testimonial (large, bold text and smaller text below in quotes) as h2s puts them too high within in the document hierarchy, especially given that theres no body content underneath them. Consider a document structure like
      • Visually hidden h1 for the page. Something like "Testimonials"
      • Names of people and their titles as wrapped in single h2s
      • Testimonial content in paragraph tags OR wrap both parts in a single blockquote element
    • The page content should be contained within a "main" landmark
    • The quote image in the first card should have an empty alt attribute to signify that it's purely decorative. Otherwise, it just adds noise for screenreaders. Consider doing the same for the thumbnails of the people; they don't add additional context for screenreaders that their name alone doesn't, so I would argue that they're also decorative.

    Card rendering thoughts I think the approach you've taken here is reasonable given the data provided. Here are some thoughts that I think might lead down some interesting paths:

    • All of the cards could be rendered in a loop and styled using "nth-child" selectors. This could be done in a way that would allow the layout to accommodate more or fewer testimonials
    • What if this data was being served from an API that you had control of, or you were in close contact with the developer that worked on the backend? Could you structure the data in such a way that specified where each item lived in the layout and/or how it should be styled?

    I hope this is useful!

    Marked as helpful
    0