Accordion challenge with javascript

Community feedback

• 

  serdarakova•260

  @Serdarq1Posted 11 months ago

  hi your project looks very good and i can give a few advice firstly i wouldnt recommend using inline-css(css inside a html tag element) nor internal(css in the head section) you should create a file just for css and then link it on your html file and the same goes for javascript

  also using display:flex will buy you so much time and also make your work so much easier for you and lastly

  i saw you wrote "item.querySelector('img')" this variable multiple times if you have a variable that you will use multiple times it would be so much better if you just define it once( const plusIcon = document.querySelector('img')) and use it as a defined version this will make your code look more readable easy to find bugs and also much cleaner

  hope that helps

  Marked as helpful

  0

  ---

  

  Jonathan Betancur•90

  @JonathanbeesPosted 11 months ago

  @Serdarq1 thanks for the advices, i would apply it in the next challenge that i will take, the most difficult part is to learn to apply the js, but everything is part of the process thank you again :)

  1

  

  serdarakova•260

  @Serdarq1Posted 11 months ago

  @Jonathanbees yes i feel you when i first started learng js it felt so overwhelming as i practiced i got better i can recommend 30-days-of-js on github, supersimpledev on yt, Jonas Schmedtmann and Angula Yu on udemy for tutorials and also do not stuck on tutorials make projects and practice as much as you can

  Marked as helpful

  1
• 

  Melvin Aguilar 🧑🏻‍💻•61,140

  @MelvinAguilarPosted 11 months ago

  Hello there 👋. Good job on completing the challenge !

  I have other suggestions about your code that might interest you.

  CSS Reset 🔄:

  • You should use a CSS reset. A CSS reset is a set of CSS rules that are applied to a webpage in order to remove the default styling of different browsers. Here can see some examples of default styles in some browsers that could potentially pose issues:

    • The body element has a default margin of around 8px, which might lead to a scrollbar.
    • Paragraphs and text elements have default margins.
    • Even if you use list-style: none;, the <ul> element reserves some unnecessary padding-left space.

    CSS resets that are widely used:

    • Reset CSS
    • "My Custom CSS Reset" by JoshWComeau

  HTML 🏷️:

  • Wrap the page's whole main content in the <main> tag.

  • You should not use inline-CSS because it is not a good practice. Instead, you should use an external stylesheet to style your page. By doing this, you will be able to have a better organization of your code and will be able to understand it better.

  I hope you find it useful! 😄

  Happy coding!

  Marked as helpful

  0

  ---

  

  Jonathan Betancur•90

  @JonathanbeesPosted 11 months ago

  @MelvinAguilar That´s a very interesting advice, thank you very much, i didn´t know that each browser has all of this properties that is better to remove o:

  0
• 

  Daniel 🛸•44,310

  @danielmrz-devPosted 11 months ago

  Hello @Jonathanbees!

  Your project looks great!

  And I have to say: Everything you've been through while doing this project is part of the learning process. Keep going! 🦾

  Suggestions for improvement:

  • One way you can align the text with the (plus/minus) image is to set display: flex; align-items: center; to the parent container.

  • Another suggestion is to place the .attribution outside the card, so it doesn't take up space unnecessarily. So you can use the space to place the 4th question that is currently not showing on your project.

  I hope it helps!

  Other than that, you did a great job!

  Marked as helpful

  0

  ---

  

  Jonathan Betancur•90

  @JonathanbeesPosted 11 months ago

  @danielmrz-dev Thanks for the advice, i will apply it the next time i need it :3

  0