Skip to content
OverviewSolutions
1
Comments
33
Bruce B
@bbsmoothdev

All comments

  • Bruce B•505
    @bbsmoothdevPosted over 1 year ago

    The responsiveness looks fine to me. Yes, you could turn it into a single column layout at narrower view points, but what you have now does work. My main feedback on this would be the heading structure. Why is $29 the main heading (h1) on the page? Don't base your headings on the size of the text. The headings should be based purely on how the content is organized. To me it is obvious that Join our community is the h1 heading. Monthly subscription and Why Us would be h2 headings. Those are the only headings on the page.

    Marked as helpful
    0
  • Bruce B•505
    @bbsmoothdevPosted over 1 year ago

    Hey, this is really nice. I am impressed with the responsiveness. I was able to set my browser view port width to 1280px and zoom in 400% and nothing broke. You also used actual button elements for the buttons which makes this keyboard accessible. You'd be surprised how many people don't do that with projects like this. And you even put an aria-live="polite" on the display. I was not expecting that. Great job!

    I do have a couple of accessibility suggestions. The primary one being that the theme switcher is not keyboard accessible. The first thing that comes to mind is to use <input type="range" min="1" max="3>. Make sure to wrap "THEME" in a label and then use the for attribute to associate it with the range. You can keep the number above the range as you have and even make them clickable if you like, but you'll want to hide them from screen readers and other assistive technology by using aria-hidden="true".

    The other thing I would recommend is using the appropriate HTML mathematical entities for the operators:

    minus: &#8722; plus: &#43; divide: &#247; multiply: &#215; equals: &#61;

    Screen readers will know how to announce them properly. Right now, the screen reader I am using (NVDA) does not announce anything for your current minus sign and for the multiply button it announces "x", which screen reader users will probably know means multiply, but why not use something to make that clearer? You should be able to plug and play all of these except the division operator, which will look more like a traditional division operator you learned in grade school instead of the slash. A good designer would make this trade-off for accessibility, but if they insist on the slash then you could use CSS to position the slash over the top of the button.

    Again, great job on this.

    Marked as helpful
    1
  • Bruce B•505
    @bbsmoothdevPosted over 1 year ago

    Hey, great start on this. I'd like you to do a little experiment with me. While viewing your page, set the width of your browser to as close to 1280px as you can get. Then, use the browser zoom feature to zoom into the page 400%. Notice that as you zoom in, the size of the text doesn't change. This is actually a major accessibility issue. To be accessible, people should be able to increase the text size by at least 200%. The way you've set up the font size is preventing people from doing that. So you'll want to let the font size grow, and the size of the card to grow to accommodate the increase in font size. To do this, I would highly recommend you not use vw units for either font size or any widths.

    Marked as helpful
    0
  • Bruce B•505
    @bbsmoothdevPosted over 1 year ago

    Wow, this is really impressive. What a cool added touch. Great job.

    I specialize in accessibility, so I'm going to give you some feedback on how to improve the accessibility of your page. Please don't take anything I write below as a negative view of your page. I really like it. I just am going to suggest some things to help make your page more enjoyable for everyone.

    First, I am very impressed with how responsive it is. The test is to set the width of the page to 1280px and then zoom in 400%. To be honest, with the added animation I was not expecting your page to handle 400% zoom but it did great. A few words were just slightly cut-off at the edge, but I could still read them. And the animation still worked as it should and I could read the content when the images popped-up. Granted, I could only see part of the images, but I don't consider that an accessibility fail since those images are purely decorative.

    Concerning those images, there is one accessibility issue with them. Whenever content appears on mouse hover, and that content covers other content on the page, then you must be able to make the pop-up content disappear without moving your mouse. This means that you need to be able to make it disappear using the keyboard. Generally you do this by configuring the Esc key to close the pop-up content. If your JS skills aren't quite up for that yet I understand. I just wanted to point this out because most people don't know about this accessibility requirement.

    And one other thing about the image animation. To be nice, it is considered a best practice to allow people to turn off the animations. There is a media query called prefers-reduce-motion that you could use to only activate the animations if the user doesn't have it set to reduce. Or you could add a switch to the top of the page that allows people to toggle them on/off. I know the design doesn't call for that, but in the real world, designers don't always get their way.

    Another accessibility issue is the heading structure. You do have one h1 near the top, which is perfect. But you have other sub-headings on the page that aren't marked up as headings in the HTML. I think "Check out our most popular courses!" is just crying out for an h2. And then all of those popular courses that follow would be h3s.

    Concerning that h1, in order to create the effect, you wrapped each letter in a pre element. I just listened to that with a screen reader and it caused my screen reader to read each letter one at a time instead of reading the actual words. Very annoying. I would solve this by having a "real" h1 heading that just has the words "Maximize skill, minimize budget" in it (without the pre elements) and visually hide it with CSS (google 'visually hidden CSS' if you aren't familiar with this concept). That way it is there for screen reader users but you won't see it on the page. And then with your current h1 you can add the attribute aria-hidden="true" to it, which will hide it from screen reader users but allow it to be seen on the page. Now you have the best of both worlds. And unless this effect depends on that being an h1, it doesn't need to be marked up as a heading at all, just use a div.

    Nice alt text on the image at the top. For the pop-up images, I would consider them all to be decorative and would give them all empty alt text (alt="").

    Again, great job with this. Enjoyed it a lot.

    Marked as helpful
    1
  • Bruce B•505
    @bbsmoothdevPosted over 1 year ago

    First, great job on this. Looks exactly like the original.

    As far as semantics, there is not always one perfect way to code this without knowing the context they are going to be used in. For example, I definitely agree that "HTML & CSS foundations" is a heading, but it would probably not be an h1. Generally, I think that a group of cards like this would be situated under a heading, which might be an h2, which would make the heading on this card an h3. Again, it depends on the context.

    Similarly, this card is most likely not going to be the only thing on the page, so you would not want to use a main in it. It's a best practice as far as accessibility is concerned to only have one main on the page. The header and footer you can keep if you want since they are inside the "real" main tag, but they are not necessary. Since these cards are meant to stand alone on their own, you could probably wrap them in an article.

    Now the fun part. Forget about the visual design of this card for a moment. If you couldn't see the information in that card but instead could only listen to it being read, don't you think it would make a little more sense to first hear the heading, and then the category, published date, and content? Especially if you had to listen to several of these cards in a row. For example, if you heard "Heading: HTML & CSS foundations" and then a bunch of information after that, you could assume that all of that information was related to "HTML & CSS foundations". And then as you continue to listen you heard "Heading: JavaScript Fundamentals", you would know that you are starting a different card and all of the information that followed would be for that new card. In other words, the heading defines when the card begins and all of the other information on the card follows after the heading.

    What I'm describing above is how a blind person using a screen reader would interact with your card. A solid heading structure and logical DOM order is one of the most important things you can do to ensure screen reader users can understand your page. Of course, the DOM order I just described does not match what you see on the page. Visually, the category and published date come before the heading, but as we just discussed, they should come after the heading in the DOM. This is where CSS comes in. You can use CSS to visually reorder these things on the page but still keep them in a more logical DOM order. So that's what I would recommend here. But please don't take this to mean I am encouraging you to visually re-order DOM items willy-nilly. No, quite the opposite, you want to try to avoid it at all costs. But there are a few specific scenarios where it is considered acceptable for accessibility's sake, and this is probably one of them. Of course, the designer of this card could have also taken accessibility into account and not put this information before the heading in the first place. But we should probably give them a break, after all, they're just designers. (I keed).

    As for the image at the top, to me this image looks purely decorative and should be hidden from screen reader users completely by using an empty alt attribute (alt=""). In this case it would not matter where you put the image in the DOM order. But if the image weren't decorative and it actually needed alt text, then just like the category and published date, it would need to be moved below the heading in the DOM and visually moved with CSS. Where would you move it? I'd probably put it right after the heading.

    As for the category "tag", we don't know since we only have this one card, but often there is a chance that there could be more than one category for a card and thus you would want to put them in a list. It would probably be helpful to provide a heading for them as well so that screen reader users know exactly what they are for. You can visually hide the heading with CSS. But if there is only ever going to be one category per card then you could probably get away with adding some visually hidden text in front of the category text so it announces as "category: Learning" to screen reader users.

    The person image at the bottom is probably decorative as well, so give it empty alt text too. The name at the very end could potentially be placed above the actual content, since usually most articles list the author before the content. But I don't think it is required. I think a screen reader user will probably realize that this is the author of the article. You could potentially add some visually hidden text before the name, such as "written by " but that may be overkill. One thing you don't want to get into the habit of doing is trying to help too much by adding a bunch of extra hidden text just for screen reader users. Any hidden text you add should be very concise and only when you feel it is absolutely necessary to help screen reader users understand your content. And of course the best thing you can do is ask people who use screen readers what they think.

    And for sure, what I've written above may not be 100% correct. I'm basing it on general best practices and experience working with screen reader users, but there often is no one perfect solution for everyone. Accessibility is a continuous process. Do your best to make your product accessible, let people test it, accept the feedback graciously, and modify as needed based on that feedback. And then start the cycle over again.

    Who knew a simple card could be so complex?

    0
  • Bruce B•505
    @bbsmoothdevPosted over 1 year ago

    My suggestion would be to use a radio button grouping in the HTML instead adding click events to spans, which is not keyboard accessible. You can use CSS to make "cover the radio buttons with the circles.

    2
  • Bruce B•505
    @bbsmoothdevPosted over 1 year ago

    Hey, overall this is great. It looks exactly like the original.

    I agree, the image icons should have alt="" to hide them from screen reader users because they are purely decorative. Your heading structure looks good now (i.e. each type of car is an h2). Yes, you should switch to single column a little sooner. And I'm still seeing a card get taller than the others as I zoom in.

    A few accessibility issues:

    • The white text on orange background does not have enough contrast to meet minimum accessibility requirements. I understand if those are the colors the design asked for. I won't hold that against you. The designer is the one to blame. Just wanted to let you know that it wouldn't pass an accessibility audit.
    • Believe it or not, the white on lighter green in the middle card does not quite have enough contrast either. It's missing by a little, so it's a minor issue. Again, I won't hold that against you.
    • The "Learn More" links at the bottom are a common accessibility issue. A screen reader user can navigate by links only. In that case they will hear just "link Learn More" and won't know which car the link is for without having to do some extra work to figure it out. The best solution for this is to use visually hidden text to add the type of car after "Learn More" for each link:
    <button class="card-button">Learn More <span class="visually-hidden">about Sedans</span></button>

    The article The anatomy of visually-hidden has all the info you need on how to visually hide text using the visually-hidden class.

    Marked as helpful
    0
  • Bruce B•505
    @bbsmoothdevPosted over 1 year ago

    Hey, just a few quick issues. You can't have spaces in your id values. So id="Question 1" needs to be id="Question1". Also, I see you are using the aria-controls attribute on each button to associate the button with the paragraph it toggles, which is great, but you forgot an even more important aria attribute, aria-expanded. This attribute goes on the button, not the p element. When the content is not showing then it should be aria-expanded="false". When the content is showing then it should be aria-expanded="true".

    Marked as helpful
    0
  • Bruce B•505
    @bbsmoothdevPosted over 1 year ago

    Hey, this is very nice and looks super close to the original. Just a few things I want to point out. Try zooming in and notice what happens to the Rules button. Also, you don't want to get into the habit of adding event handlers to divs. Because of this, I can't use the keyboard to choose a hand. Those should be HTML buttons.

    Marked as helpful
    0
  • Bruce B•505
    @bbsmoothdevPosted over 1 year ago

    This looks very nice. I like the animation on the results. A few suggestions for accessibility improvements. You can't use the keyboard to play this game. That's because you used divs instead of buttons. Don't make it a habit to add event handlers to divs. If you are adding a click handler to an element, then it should be an element that can be clicked by both the mouse and keyboard, such as a button.

    Also, widen your browser to 1280px (it can be approximate) and then zoom in 400%. You'll see that you can't access the score. You'll want to fix that. Many people with bad vision will need to zoom in that far in order to see the page. They need to be able to see the score as well.

    1
  • Bruce B•505
    @bbsmoothdevPosted over 1 year ago

    This looks really good. I think you are triggering the mobile layout a little too early though. I have to make my browser pretty wide before I can see the desktop version. Also, I think you are missing a heading. You are using an h2 for the Summary heading, which is perfect. But that's it. There is definitely an h1 heading. What do you think it is?

    0
  • Bruce B•505
    @bbsmoothdevPosted over 1 year ago

    The accessibility on your toggle buttons is great. But I would double check if you have the right values for the aria-hidden attribute on the content.

    Marked as helpful
    0
  • Bruce B•505
    @bbsmoothdevPosted over 1 year ago

    Great job. This looks just like the original. Just a few recommendations for you to consider.

    • You didn't use any headings. I definitely think there are headings on this page. Where do you think they are? Headings are very important for people who can't see your page and are using a screen reader to listen to your page.

    • The responsiveness is almost perfect but has some issues at the edges. One test you should always do is to set your browser width to 1280px and height to 1024px and then zoom in 400%. At that zoom level you should be able to read all the content on the page without having to horizontal scroll. As you will see, there is a slight horizontal scroll at 400%. The alternative is to use the dev tools responsive design mode to shrink the view port down to 320px width by 256px height with normal 100% zoom. This will give you the same proportions as the 400% method.

    • I will add that the design is using inaccessible color combinations, such as the light gray text on purple. I'm not going to knock you for that since you are just following the design specs. But in the real world you would need to make the text darker so that it meets minimum contrast ratio requirements.

    Marked as helpful
    1
  • Bruce B•505
    @bbsmoothdevPosted over 1 year ago

    Question for you. How are people who can only use a keyboard going to be able to see the dollar amount for each day? I'm not expecting you to solve this issue. I'd say it's a flaw in the design. But I just wanted to make you aware that things like this need to be accessible to people who can only use keyboards to navigate your page. So using just mouse hover to show the dollar amounts won't work.

    There are a number of ways to solve this. Personally, I would probably include a table of the data. You can hide it in a disclosure below the chart so that it can be displayed on demand when needed. But this would deviate from the design, which is why I suggested that this is a design issue.

    1
  • Bruce B•505
    @bbsmoothdevPosted over 1 year ago

    Some accessibility help. You shouldn't add event handlers to non-functional elements because keyboard users won't be able to access them. The element that shows/hides the content needs to be functional. I would suggest a button. See Tutorial: Let's Build an Accessible Disclosure for an example of how to do this.

    Marked as helpful
    0
  • Bruce B•505
    @bbsmoothdevPosted over 1 year ago

    Another suggestion. You shouldn't add event handlers to non-functional elements such as h3 because keyboard users can't access them. I would recommend you use a button for the questions and then add the click handler on the button.

    Also, you'll need to work on the responsiveness a little. Always perform the following test. Set your browser width to 1280px and the height to 1024px and then zoom in 400%. If you do that I think you'll see some issues that need to be fixed.

    Marked as helpful
    1
  • Bruce B•505
    @bbsmoothdevPosted over 1 year ago

    Great work. It looks really nice and functions properly. I have some quick accessibility fixes for you.

    The CSS:

    button {
  all: unset;
}

    Removes the default keyboard focus outline from the button. So you'll need to add it back. Focus outlines are extremely important for keyboard users and are a requirement for accessibility. I would recommend you use the :focus-visible pseudo-class to style the button's outline property.

    You've added a click event to each team card, but you added it to a non-functional div, so keyboard users won't be able to click on a card because a div doesn't get keyboard focus. Instead, use an accessible card pattern to make the entire card clickable while still being accessible to everyone.

    Marked as helpful
    0
  • Bruce B•505
    @bbsmoothdevPosted over 1 year ago
    .btn {
  outline: none;
}

    You should never remove the keyboard focus indicator. People who can only use a keyboard to access your page rely on that. Removing the focus indicator is basically the same as using a mouse but not being able to see the mouse pointer.

    If you don't want mouse users to see the focus indicator then you can style :focus-visible, which will only show up when someone is using a keyboard. In fact, that's what all the browsers use by default now. But when you set outline: none then you remove that completely.

    You've also removed it for the inputs. That's not quite as bad because you will still see the blinking text cursor. But really, you should have a focus indicator for the text inputs as well. Yes, even if you use :focus-visible the outline will still show up for mouse users. Just make it look cool and that will be OK.

    But if you don't want to create your own focus outline styles then just remove outline: none and rely on the browser defaults. It won't be quite as good as rolling your own, but at least it will be something.

    Marked as helpful
    0