Animated Article preview component

Community feedback

• 

  T

  Gwenaël Magnenat•1,290

  @gmagnenatPosted 5 months ago

  Hi, congrats for completing the challenge, your solution looks great ! I worked on the same challenge recently following the JavaScript learning path and yours was picked up for a review.

  I did some tests on your solution and checked your repository. I can give you a few comments to improve this solution or to open a discussion on it.

  I like the css animations. that's cool !

  • The drawer image should have a meaningful alt text. I don't think this is purely decorative.
  • The avatar image usually are meaningful as well and need an appropriate alt text
  • I wonder if the aria-haspopup is appropriate on the button as you already use a disclosure with aria-expanded
  • You use a menu role and menuItem on the disclosure content. Not sure this is appropriate as it ads a lot of verbosity and misleadinging informations when using a voice over. It should be more concise like "share this content on Facebook". For example it mentions a close interaction with the escape key that doesn't work on your solution. Probably something to check and confirm.
  • I don't think you need two copies of the button. You can use a single one and position it over the disclosure content on mobile.
  • In general in the css, I would recommand to use a full modern css reset at the beginning
  • I recommand to use more meaningful variable name instead of a, b, c, d and 1, 2, 3, 4. It will increase the readability, make it easier to debug.
  • You could use the aria attribute to style the disclosure instead of adding open and close classe to simplify your css and javaScript.
  • Some interactions are missing for me here. The only option to close the disclosure is by clicking the button a second time. I would like to have the "escape key" option and a light dismiss (when the user click an area anywhere but the button).
  • There is an issue with the button to close it. If you click on the svg pattern the click is not handled. Maybe something with pointer-events:none on the svg can fix this?
  • Still on this share button, the svg is purely decorative. By default it is brought in focus by some screen reader and voice over. It just reads "image" without any more informations. To avoid this, add focusable="false" to the button's svg.

  I hope you find these comments helpful and it helps you improve this solution to make it even more accessible. Reach out on discord if you have any questions or want to open the discussion on these comments.

  Happy coding !

  Marked as helpful

  0

  ---

  

  Alberto Raúl José•610

  @alberto-rjPosted 5 months ago

  @gmagnenat thank you very much for your great suggestions, my dear friend, for sure your great comments will help me to significantly improve both this solution and my future projects.

  As for discord, yes, I'm also totally open to discussing topics like this and connecting with more experienced professionals in the same field.

  0