Challenge #2 - FAQ accordion by Pau Ferrer

Community feedback

• 

  T

  Grace•31,230

  @grace-snowPosted over 3 years ago

  Hi Paul,

  Definitely use buttons in this to add keyboard accessibility.

  Also, I recommend you read up more on alt text, how to write alt descriptions on images and when they are/are not needed. At the moment, this is less than ideal for screenreader users.

  Other than that though, this looks really nice, very close to the design and nice transitions

  1

  ---

  

  Pau Ferrer•85

  @PaulOCastlePosted over 3 years ago

  Thanks @grace-snow!! I definitely will be changing the code to use the buttons!! Also, thanks for pointing at the importance of the alt text, I will look into it and will improve my coding to be more screenreader friendly!! BTW, did you check the JS code?? I'd love some feedback on it. Thanks again!!

  0

  

  T

  Grace•31,230

  @grace-snowPosted over 3 years ago

  @PaulOCastle the js looks fine and works OK but could definitely be a lot simpler.

  I usually advise against manipulating inline styles with javascript. Instead, I would do everything with css. All the js needs to do is toggle one class on each collapsible. Wrapper element. Just adding an is-open class (or changing that data attribute) would be a hook you can use in css to do everything else you need.

  Altering height is a performance heavy change as it triggers a dom repaint (as opposed to opacity or transform, which do not). But I don't think there's any way around that if you want a nice effect on a collapsible element. I would still change height on the faq content, but from 0 to auto rather than setting a pixel value (again, there is no need to set the height with js, let css handle it)

  1

  

  Pau Ferrer•85

  @PaulOCastlePosted over 3 years ago

  Hi @grace-snow, Thanks for that, I definitely didn't got it right then, as in my search on how to code the JS for the collapsible object, I found that height is not possible to transition unless the specific height (in px or rem, or any other size measurement) is set. And transitioning to auto didn't work either, so I definitely didn't understand nor found the correct advice up to date. Every site I looked into mentioned that to do a collapsible element that would vary its height, the only way to have a correct transition is by injecting inline styles with JS. Please, if there is any web or place you know I can learn how to work this "problem" around I would be very pleased!! Thanks for all you help and feedback, I am really learning lots!!

  0

  

  Pau Ferrer•85

  @PaulOCastlePosted over 3 years ago

  Hi again @grace-snow!! Just letting you know that I have done the changes to use the button and to improve the alt attributes. Please, don't get me wrong or as a problematic person asking all the time. I really appreciate all you inputs and will continue to if you keep giving me feedback!! Do not have to check the code again, but just letting you know I have done it. I'll definitely will look into the JS and the setting the height using CSS/SCSS only and not as an inline style injection. Thanks again!!

  0

  

  T

  Grace•31,230

  @grace-snowPosted over 3 years ago

  @PaulOCastle take a look at Heydon pickerings inclusive components, see if that helps with anything. His patterns are usually very good

  1