I’ve just finished my 4th project on responsive web design I I’m quite proud of the result without any framework or JS (even though I’d used in the beggining to help me with a height property that was not working as expected, but later I’d found out why).
My goal was to make a landing page for a HyperX product, the solocast. Very simple, but with some nice responsiveness features and a toggle bar in my navigation with pure CSS.
The CSS code may have gotten quite big and there might be some unecessary divs, but it’s working.
Could you guys take a look and give me your feedback, code and design
Overall this is very nice. Good responsiveness. I like that you’ve put a max-width on the outermost div instead of trying to define various widths using media queries. Your hamburger menu does work quite well for only using CSS. The image links in the header all have alt text describing where they point to (lots of people forget to do this).
Most of the things I’m going to point out below are related to accessibility (since it’s sort of my thing).
The “Learn More” link is marked up as a button when it should be a link. The basic rule is that “links go places” and “buttons do things.” Mark it up as a link and if you want it to open in a new window then use target=_blank" (although a lot of accessibility experts will tell you not to force new windows on links like this). This will allow you to get rid of the onclick handler. Also, since you weren’t submitting a form with this button then the value attribute wasn’t doing anything. And links don’t have a value attribute, so just get rid of that completely.
Further, links with text like “learn more” aren’t considered very accessible, especially when they sit alone outside of other content. That link should probably say something like “Learn More About the New HyperX Solocast”. Of course, that seems a little redundant if you can see the text right above it. So what you would do is visually hide the text after “Learn More” so that screen reader users will hear the whole thing but people looking at the page will only see “Learn More”.
The email input needs some fixing. The label should describe the information you want the user to enter in the input. This is important so that when someone who can’t see your page and is listening to it instead activates this input they will know exactly what you want them to type in there. So the label should probably just be “Email address” (or “Email”) and sit either above or to the left of the input. The current text you have in the label is just content that should probably be in a <p>. I know the placeholder text is required for this challenge but normally I don’t think you’d need it because I think it is obvious what an email address is. But since you are required to use it here then it should be an example email address (“firstname.lastname@example.org”).
The “Partnership” text looks like a heading to me and should be marked up as a heading (<h2>).
I’m not sure the “Level up your game with HyperX” heading should be a heading. It’s not styled as a heading and it comes immediately after the <h1> which says to me that it is content for the <h1>. I’m thinking it should probably be a <p> but I could be convinced otherwise with the proper argument
You’ve set the image alt text to null for the top image, which is fine if the image is purely decorative, but in this case I would argue that it isn’t and you should add some good descriptive text for this image. Especially since it is the primary image on the page and there really is no actual content on the page that describes the product, so your image alt text needs to do that. I know you have alt text on the image at the bottom, but it’s at the bottom, so I think it is much more important that the image at the top have alt text since that is most likely the first image a screen reader user will hear.
This is very picky, but I would also try to put the image after the <h1> in the DOM if possible to help screen reader users find it since they often navigate a page by headings and might miss it since it is currently in between the nav and the h1.
The logo at the top seems purely decorative to me and the current alt text isn’t very helpful to screen reader users. Since it’s not being used as a link I think this can safely have empty alt text. Unless you think it is important that screen reader users know what the logo looks like, then you need to describe the logo in the alt text, not just say that it is the logo.
The red skinny little “HyperX” above the “Learn More” link is very hard to read, especially when it is over the image. And the white isn’t much better. I think this needs to be in a heavy font. You might possibly have to put an opaque background behind it as well.
Please don’t take any of the above as an indication that I don’t like your page. I certainly do, you did a fine job. I just get real excited about accessibility and so can’t help but point these things out.
Thank you very much for your feedback and your time spent on it! I basically agree with all your arguments about accessibility. They are very informative and insightiful. Eventually, I might refactorate the website and will take everything in consideration!
Just one question, what do you mean by “It’s not styled as a heading”:
I’m not sure the “Level up your game with HyperX” heading should be a heading. It’s not styled as a heading and it comes immediately after the <h1> which says to me that it is content for the <h1> . I’m thinking it should probably be a <p> but I could be convinced otherwise with the proper argument
How it should be styled to be a heading? And a subcontent of <h1> shouldn’t be <h2>?
Headings on a page are usually bigger than the rest of the content. They stand out as markers as to how the page is organized. That text is some of the smallest text on the page, so it doesn’t look like a heading.
An immediate subheading of an <h1> should definitely be an <h2>. My argument is that the text is not actually a heading. Headings introduce content. This text seems to be the content for the <h1>. Is it an accessibility failure to leave it as an <h2>? No. But I think that it’s probably overkill to have it as a heading.