Responsive Faq According Card

Community feedback

• 

  P

  Christopher Adolphe•600

  @christopher-adolphePosted over 2 years ago

  Hi Naveen, nice try for this challenge. 😉

  The questions you've posted along with you solution caught my attention and after analysing your code, I think I can provide you with some useful feedback for an overall improvement.

  HTML:

  Though your solution is visually close to the design, you could improve your html code structure for better scalability and reusability. You could refactor it in the following way:

  • Move the <section> element inside the <main> and eliminate the <Illustration/> component.
  • Create two components <Card/> and <Accordion/>
  • Then update your App.jsx file like this:

  <div className="App">
    <main>
      <section>
        <div className="container">
          <Card illusUrl={ illustrationUrl } title={ title }>
            <Accordion items={ faqData }/>
          <Card/>
        </div>
      </section>
    </main>
    <Footer/>
  </div>
  

  • Then your <Card/> component will contain the illustration one side and the <Accordion/> component projected on the other side using { props.children }, like this:

  <div className="card">
    <div className="card__illustration">
      <img src={ illusUrl } />
    </div>
  
    <div className="card__content">
      <h3 className="card__title">{ title }</h3>
  
      { props.children }
    </div>
  </div>
  

  • Then refactor your <Accordion/> component like this:

  <div className="accordion">
    {
      items.map(item => (
        <div className="accordion__item">
          <div className="accordion__header">
           <button className="accordion__toggle">{ item.title }</button>
          </div>
  
          <div className="accordion__body">
           <p>{ item.text }</p>
          </div>
        </div>
      ))
    }
  </div>
  

  • And finally update your <Footer/> component like this:

  <footer className="attribution">
  Challenge by <a href="https://www.frontendmentor.io?ref=challenge" >Frontend Mentor</a>.
  Coded by <a href="https://github.com/Naveen39O">Ongole Naveen</a>.
  </footer>
  

  By setting your React components as mentioned above, <Card/> and <Accordion/> are no more tightly coupled and hence more reusable as both components are receiving their content via props. The <Card/> component can be even more scalable if you add checks for the its props like this:

  <div className="card">
    {
      illusUrl ? {
      <div className="card__illustration">
        <img src={ illusUrl } />
      </div>
      } : null
    }
  
    <div className="card__content">
      {
        title ? (<h3 className="card__title">{ title }</h3>) : null
      }
  
      { props.children }
    </div>
  </div>
  

  If illusUrl and title are not set, then a simplified <Card/> component will be rendered with only the projected content. That's cool right ? 😎

  SCSS:

  • Please consider reworking the responsive styles for the illustration because I noticed that on smaller viewports, it sits right on top of the accordion and this affects the user's experience.
  • I noticed you achieved the placement of the illustration and the accordion by using huge margins and paddings. This is not practical at all. However, by refactoring your HTML structure, you can use the <div className="container"> to set a maximum width and then style the <div className="card__illustration"> with respect to its parent <div className="card">. Using the display: flex; will allow you to switch from a vertical to a horizontal layout easily rather than adjusting these huge margins and paddings.
  • You have used the clip-path css property in conjunction with position: absolute; on the illustration to achieve the offset on desktop, good job on that 👍. However, when you'll refactor the styles, please make sure to set the <div className="card__illustration"> to position: relative; so that img positions work with respect to its parent.
  • As for Sass, it provides you with great features like nesting, mixins, inheritance etc which eases the maintenance of your stylesheets. However, on such a small project you'll probably not see much gain in using it but as your project grows, a good Sass structure will make your development experience better. I can see that you have done a good job by creating partials for colors.scss, mobile.scss and desktop.scss 👍. If you refactor your HTML code as mentioned above, you can create new partials for _card.scss and _accordion.scss which will contain your css rules for the respective components. For instance, your _accordion.scss partial will look like this:

  .accordion {
    // your Block rules here...
    
    &__item {
      // your Element rules here...
    }
    
    &__header {
      // your Element rules here...
    }
  
    &__toggle {
      // your Element rules here...
    }
  
    &__body {
      // your Element rules here...
      &--is-collapsed {
        // your Modifier rules here...
      }
    }
  }
  

  Here we are combining the nesting feature provided by Sass with the BEM methodology which helps you write more structured css. Unfortunately, I cannot elaborate on Sass structure and css methodology here else the review will never end 😆 . Here are some great articles that I highly recommend you so that you can level-up your Sass game. 💪

  • Sass Basics
  • Introduction to BEM
  • Sass Style Guide
  • A Simple SCSS Architecture, and Best Practice Playbook
  • Structuring your SASS projects

  NOTE: This is definitely not an exhaustive list but it is a very good starting point.

  Accordion Toggle Logic:

  • Regarding your logic in the handleClick() function, accessing DOM elements directly by id, class name or element tag is considered as bad practice in React. React uses a virtual DOM and tracks changes to update this virtual DOM. Anytime there is a change, React takes to responsibility to keep the real DOM in sync with this virtual DOM. Whenever you access real DOM elements directly, you bypass React's synchronisation and this can potentially lead to undesired behaviours. You should instead aim to update React's virtual DOM and you can achieve this using the useRef hook. Read more on useRef here.
  • An easy alternative solution would be to toggle a class on the accordion body programmatically in the Accordion.jsx file as follows:

  import { useState } from  'react';
  
  function Accordion(props) {
    /*
    * Creating an `accordion` state by mapping over the `items` props
    * and adding a new property `isCollapsed` which we can then use to
    * toggle the class on the accordion body to create the hide/show
    * behaviour
    */
    const [ accordion, setAccordion ] = useState(props.items.map(item  => ({ ...item, isCollapsed:  true })));
  
    const  handleClick = (index) => {
      setAccordion(prevState  => {
      const  currentItems = [ ...prevState ];
      const  updatedItem = currentItems[index];
      updatedItem.isCollapsed = !updatedItem.isCollapsed;
      return  currentItems;
    });
   };
  
   return (
   <div className="accordion">
    {
      accordion.map((item, index) => (
        <div key={ index } className="accordion__item">
          <div className="accordion__header">
           <button className="accordion__toggle" onClick={ () => handleClick(index) }>{  item.faq  }</button>
          </div>
          {/* Using the `isCollapsed` property to toggle the `accordion__body--collapsed` class */}
          <div className={ `accordion__body ${item.isCollapsed ? 'accordion__body--is-collapsed' : ''}` }>
           <p>{ item.answer }</p>
          </div>
        </div>
      ))
    }
   </div>
   );
  }
  

  Where to go from here: I know this might look like a lot to take in but I better do it well or not at all. 😉 I think you should take your time and go through these suggestions and give it a second try. If you are not yet at ease with React, I would also suggest you to try doing the challenge with plain HTML, SASS and JavaScript. I sincerely think that you master these 3 first and then move to frameworks/libraries like (React, Angular or Vue) because only then you will unlock the potential of these tools.

  I hope this helps you.

  Happy coding.

  Marked as helpful

  1

  ---

  

  Naveen Ongole•80

  @Naveen39OPosted over 2 years ago

  @christopher-adolphe Thanks for taking time and giving detailed review.

  0

  

  P

  Christopher Adolphe•600

  @christopher-adolphePosted over 2 years ago

  @Naveen39O I'm happy to help.

  You can contact me if ever you need any clarification.

  Best regards.

  0