Hi
I've had a look through your solution and hope this feedback helps! It looks good overall, but there are some improvements to suggest
- This should have a header and main landmark. Header cannot go inside main, they have completely independent roles
- Try not to repeat html. There is no reason to have two lists of nav links in this. You can use CSS to style them so they work as a revealed element on mobile and a persistent one on larger screens
- The mobile menu toggle button needs an accessible name and an
aria-expandedattribute with the boolean value toggling on click. This is essential. - This challenge needs some visually-hidden headings to make the content make sense. You can inspect the BBC News homepage and you'll see that approach implemented. For example, the h1 for this page would be "News Homepage", then a h2s on the page might be "Featured", "New" (that one is visible in the design) and "Popular", then all article names would be h3s
- You shouldn't wrap parts of the featured article heading in spans. If you want to make the lines break at specific places, consider using a max-width in ch units instead. (Spans will be announced as "group" by some assistive technology, which can really break the flow when reading)
- Well done on adding the links to the New articles! Loads of people forget to add those. Technically, those anchors should be inside the h3s, and you seem to have forgotten the anchors off the posts in the bottom section
- I think the bottom section should be an ordered list
- I think the images on this should have alt descriptions
- Once you've adjusted the headings, the heading levels of the bottom section's articles should be h3s not h4s. Remember never to jump heading levels. For those to be h4s they would need to "belong to" the previous h3 (which says Is VC Funding Drying Up?)
- Consider using gap to create space between flex children instead of margin
- At small screens the mobile menu is becoming visible even when closed. That's because it's
right: -70%;when it should beright: -100%; - The mobile nav doesn't work when text is enlarged or on mobile landscape because you've used overflow hidden on it. That's going to be tricky to solve, but will probably involve using position properties and position fixed somewhere; plus overflow-Y auto and min-height only on one layer of the nav; and overflow hidden on a different layer of the DOM somewhere. I've managed to do this successfully before but I'm struggling to work out what you need looking in devtools (sorry!). It might be worth looking at other sites that have the pattern and reverse-engineering them via devtools to work out what you should do with it. I think the modal--active way you are doing that overlay is partly to blame - Usually you create that dark overlay by adding a pseudo-element on the body, which means it can have a totally separate stacking context to the revealed content inside the nav. Other alternative options are to not disable scroll at all when the nav is open, or make the header sticky so scrolling would bring the nav along on scroll. Those may cause new issues though. Sorry I haven't got more concrete help on this one!
Marked as helpful
@oshudev
Posted
@grace-snow the reason I have span on my h1 is because the letter spacing of Future of and Web 3.0? is different. I replicated the provided design and based on that they have 1% difference in terms of letter spacing. How can I have different letter spacing in specific characters without using span? Other than that I will try to implement your suggestions before moving to another challenge. Thank you for the feedback!
@eurus1108 I've checked the figma and cant see any difference in letter spacing. And in a real project you would never have that level of granular control anyway as content is managed separately. We create slots for the text to go into so can't add spans or brs (unless we target with javascript to change the color of a word or something - that is rare but does happen occasionally)
Marked as helpful
