Manage Page - SCSS and JS (Siema for Carousel)

Community feedback

• 

  T

  Grace•31,530

  @grace-snowPosted over 3 years ago

  One more thing I thought of actually, triggered by @tediko's comment. Look into using the current="page" attribute and value in your navigation

  1

  ---

  

  P

  Dave•5,295

  @dwhensonPosted over 3 years ago

  @grace-snow thanks! Added to the list of things to update! I actually used to include this one but seemed to have forgotten about it!

  1
• 

  P

  tediko•6,680

  @tedikoPosted over 3 years ago

  Hello, Dave! 👋

  Congrats on finishing another challenge! Yet again, you did great job! Nothing to add after Grace exhaustive feedback, but hop in to say some kind words. Maybe small tip is that to make your alternative text/aria-labels more descriptive. For logo - "Manage - home page" and for social icons "Follow us on XXX" etc.

  Good luck with that, have fun coding! 💪

  1

  ---

  

  T

  Grace•31,530

  @grace-snowPosted over 3 years ago

  @tediko you don't need "follow us on..." - screenreader users don't like extra bloat. If an icon is enough for everyone else, the name of the destination is enough for assistive tech 😉

  1

  

  P

  Dave•5,295

  @dwhensonPosted over 3 years ago

  @tediko thanks for the kind words! Always appreciated!! I had a goog poke around your IP tracker solution- really nice!

  1

  

  P

  tediko•6,680

  @tedikoPosted over 3 years ago

  @grace-snow Thank you for correcting me. I've read that more descriptive titles are good but i see your point. In the case of the logo, when the name of the page itself will not tell the user what will happen after interaction it is good to add some extra info on it but on the other hand in the case of socials links when Facebook/Twitter are self-defining there is no need to add more to it. Noted 😊 Fortunately, it's not harmful and the people I gave this advice won't be angry at me 😅

  1

  

  P

  tediko•6,680

  @tedikoPosted over 3 years ago

  @dwhenson Thank you! That means much to me :)

  0
• 

  T

  Grace•31,530

  @grace-snowPosted over 3 years ago

  Hi Dave,

  This looks sweet, well done! 👏

  The reason your anchor tags aren't focusing is they are missing the href attribute

  Looking at your report for this, you need to change some other attributes too. If you want to add custom attributes you should be using data- to prefix those attributes (or are things like status and enabled part of the third party carousel maybe?)

  I agree, carousels are poor for UX and accessibility. Sadly, sometimes clients will insist on them. Heydon Pickering has written about them on Inclusive Components, so you might find that helpful: https://inclusive-components.design/a-content-slider/

  I don't think you need any of the aria-labelledbys on your sections. I tested this on a project of mine recently and found it just added bloat of extra announcements and didn't help me navigate the page with my screenreader. Those sections all have headings anyway. If you thought they needed extra affordance and the content is able to standalone, you could make them into articles if you wanted, but that's not essential either.

  On your footer, only what you have as nav-inner should be in a nav element. (At the moment you have links to external social media inside that footer nav). Maybe consider using column style properties and placing all those footer links into one ul instead of two as well.

  I think you need to add a flex-wrap somewhere in your footer too (.footer > .container.split). At the moment, as I shrink the screen down, at some point the footer content breaks out of its container.

  With the mobile menu, you are nearly there... You just need to add aria-controls to the burger-menu__trigger button and an ID on the burger-menu__panel so those are linked to each other. At the moment it says it's expanded, but doesn't say what it controls.

  really nice work on this, again. I hope the feedback helps

  1

  ---

  

  P

  Dave•5,295

  @dwhensonPosted over 3 years ago

  @grace-snow thanks so much! I'll go thought the points you raised - really appreciate all the advice.

  Can't believe it was because I was missing the href! Note to self, check the HTML first next time!!!

  One last follow up question if you have a moment: why should the external links to social media be outside the nav? Are they not considered part of the navigation?

  Actually, forget it as I type that sentence, I can see they are not!

  Thanks again.

  0

  

  P

  Dave•5,295

  @dwhensonPosted over 3 years ago

  @grace-snow Thanks again for the inputs. On the burger-menu, I'm basing this on a tutorial Andy Bell put out so I thought I'd ask him if he thought about adding aria-controls as the tutorial was really in-depth. He actually responded! And said:

  "that's a legacy role now the relationship is implied with the existing relationship in the DOM of trigger and panel. If they are far apart, it wouldn't hurt to put aria-controls on there, I suppose. I haven't come across it for a long time, personally".

  I don't have any basis for an opinion here, but I just thought you'd be interested in his response. I am finding that this a11y stuff seems to be much more nuanced and harder to get solid consensus on that other areas I'm learning.

  1

  

  T

  Grace•31,530

  @grace-snowPosted over 3 years ago

  @dwhenson yeah that's really interesting. I don't find my screenreader does anything different with them but figured it's just a setting I haven't turned on. I'll have to look more into this one I think. Thanks

  1

  

  T

  Grace•31,530

  @grace-snowPosted over 3 years ago

  @dwhenson whoaaah I ma not gonna bother with those attr any more I think 😂 https://heydonworks.com/article/aria-controls-is-poop/

  1

  

  P

  Dave•5,295

  @dwhensonPosted over 3 years ago

  @grace-snow ah, great! Glad the info was useful!! Heydon's article is great thanks. I like his writing.

  0