Accessible Accordion with Pure JavaScript
Design comparison
Solution retrospective
I couldn't get the <main>
element to correctly center horizontally on the screen, it is slightly offset to the right. This is especially noticeable on small screen widths. If anyone knows why this happens I'd be grateful to learn it.
Community feedback
- @Islandstone89Posted 10 months ago
It looks OK for me, too. But I don't recommend setting a fixed width, that usually does more harm than good. So I would at least change
width
tomax-width
- this will allow it to shrink if needed.A few other things I noticed...
HTML:
-
Headings must always be in order, so you should not jump from
<h1>
to<h3>
. Hence, I would change all instances of<h3>
into<h2>
. -
Footer text must be wrapped in a
<p>
.
CSS:
-
You don't need to give the
body
a width of100vw
, as that is its default width anyway. -
Similarly, the default root font size is
16px
, so no need to declare it. Remember, though,font-size
must never be in px. This is bad for accessibility, as it prevents the font size from scaling with the user's default setting in the browser. Use rem instead. -
It is common to do mobile styles first, as its layout is usually less complex. This means setting min-width media queries, and they should be in rem.
-
You are giving
main
abox-sizing: border-box
on smaller screens.box-sizing: border-box
is usually set on all elements, like this:
*, *::before, *::after { box-sizing: border-box; }
This is usually done at the top of the stylesheet, as part of a CSS Reset. I would recommend including it every time - I use this CSS Reset by Andy Bell.
Marked as helpful1@tenze21Posted 10 months agoIs it ok to use the header tags like it is done here for styling purpose because i think the developer used the h3 tags to make the text a bit bold but the contents don't actually seems to be header. Just something that struck me while going through the code. @Islandstone89
0@Islandstone89Posted 10 months ago@tenze21 No, you cannot skip headings. You can style it in CSS if needed, but the HTML document needs to have headings in order.
0 -
- @ClonephazePosted 10 months ago
Solution looks good! I looked at the page, played around with it in responsive design mode at various sizes, and looked at your code for centering. Everything seems fine, on my screens everything is properly centered. Maybe a scrollbar was popping up and shifting things?
My one recommendation would be to set your width to something more like "width min(350px, 90vw)". min() is a fun one, allowing contents to adjust for different conditions. It was always choose the smallest value to work with, in this case it would allow your main content to take up to 90 percent of the width of the screen but never appear smaller than 350px. Responsive widths like that are great for designing content that can adapt to various screen sizes without needing to specify widths at specific viewport sizes.
Marked as helpful1@high-rollsPosted 10 months ago@Clonephaze thanks, that's a great idea to use the min function! I should learn more about the core functions in CSS, I only know a few of them.
0
Please log in to post a comment
Log in with GitHubJoin our Discord community
Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!
Join our Discord