FAQ accordion card

Community feedback

• 

  SutonToch•340

  @SutonTochPosted about 1 year ago

  I didn't think I'd come back so soon, but it didn't let me rest, so here we are.

  I only did very light reading on jQuery, so take my notes with a grain of salt. But my notes will most definitely make your code more readable.

  So let's get cracking:

  • Your first and second if-clause repeat each other, in that both want to close the currently open dropdown. In those cases, it's always best to create a function that handles that part for you, e.g.:

  function closeDropdown(activeDropdown) {
    $(activeDropdown).removeClass("active-dropdown");
    $(activeDropdown)
      .find(".container_right_dropdown_arrow")
      .removeClass("rotate-arrow");
    allTexts.each(function (i) {
       $(allTexts[i]).hide(300);
    });
  }
  

  • You could do the same thing for opening a dropdown, just to stay consistent. E.g.:

  function openDropdown(clickedDropdown) {
    $(clickedDropdown).addClass("active-dropdown");
    $(clickedDropdown)
      .find(".container_right_dropdown_arrow")
      .addClass("rotate-arrow");
    allTexts.each(function (i) {
      if ($(allTexts[i].parentElement).hasClass("active-dropdown")) {
        $(allTexts[i]).show(300);
      }
    });
  } 
  

  • Let's tackle the allTexts.each(...) next. With that you loop through every single text, to find the correct one, and then show/hide it. We don't need to loop everything, because we already have the parent. To show the text when opening, you could write the following instead:

  $(clickedDropdown).find(".container_right_dropdown-text").show(300);
  

  • The great thing is, you already used the .find() method for the arrow, so we're staying consistent. Another bonus, we don't need the global constant allTexts anymore. You can do something very similar with closing the texts too.
  • Let's address your second if-clause next, where I can only assume that with $(dropdown).hasClass("active-dropdown") jQuery goes through every single .container_right_dropdown_each-faq and looks if it is the active dropdown. It would be more efficient if we would store the active dropdown in a global variable. This way we could close it directly using closeDropdown(activeDropdown).
  • Obviously, a few tweaks are needed for that to work. When opening a dropdown, that clicked dropdown must be saved to the active dropdown. When closing a dropdown, the active dropdown must be removed (e.g. reassign with null).
  • In your first if-clause, we can now also check if the clickedDropdown is our activeDropdown via $(clickedDropdown).is(activeDropdown).
  • Now our internal logic when a dropdown element is clicked looks more like this:

  dropdown.click(function (e) {
    // saves the element clicked in a variable
    let clickedDropdown = e.currentTarget;
  
    if ($(clickedDropdown).is(activeDropdown)) {
      // Clicked on the only open dropdown
      closeDropdown(clickedDropdown);
    } else {
      // Clicked on a different dropdown, than the open one -> close the original open one
      closeDropdown(activeDropdown);
  
      // Open the clicked dropdown
      openDropdown(clickedDropdown)
    }
  });
  

  • Puh.. this was probably a lot to take in. I'd recommend retracing my written steps in your own pace again.

  A few takeaways:

  • Think about what functionalities you want to implement (e.g. opening and closing a dropdown) and write a function for those.
  • Then create your logic, that calls the functions with the correct parameters when they are needed.
  • Make use of the information you already have (e.g. the clicked element and therefore the text of its child)
  • Usually that's an iterative process, because it's very rare that the original plan pans out exactly like one hoped, or you find an even more efficient way.

  I hope I was able to help you with this :), but don't forget that it's very normal for the first iteration of code to "just work". I'm pretty sure my proposed improvements can be improved even further. Feel free to ask me any further questions, I'm always happy to help.

  Keep on going!

  Marked as helpful

  1

  ---

  

  OsolalDev•70

  @OdaSolaDevPosted about 1 year ago

  Hey there, @SutonToch

  Both of your comments were really helpful. Especially the one for jQuery. My code for this challenge (and I hope for the ones to come too) is much clearer now.

  You made REALLY good notes about my code.

  Thank you! You actually helped me A LOT. ^^

  0
• 

  SutonToch•340

  @SutonTochPosted about 1 year ago

  Congrats! I'm glad that this challenge gave you some trouble :P because I had a lot of trouble.

  Again, I really like your comments, makes everything so much more readable. I need to do that too.

  Your positioned elements look fine to me. I can't really comment on the js, because I still need to learn jQuery, but it looks good, for whatever that's worth. I'll come back to it, though.

  Small notes:

  • Currently, if you visited the webpage on mobile, the desktop images are loaded too. With this few images, it doesn't really make any difference, unless you have a really slow internet connection. To solve that, you could take a look at the <picture>-element.
  • Maybe consider adding a transition: transform 0.3s; for your arrow-icons, but that's a style choice and not really needed because of the really nice show and hide animations of the text.
  • I don't think transition: 0.3s; on .container_right_dropdown does anything. Probably an artifact before you added the animation via jQuery.
  • Consider adding :focus to your :hover element too, for people that tab through webpages.

  In terms of styling, the font-size could be a little bigger and the box-shadow a little more prominent. Other than that, it's perfect.

  Well done! This is a really nice solution. And I'll definitely come back for the jQuery :).

  Marked as helpful

  0