0
\$\begingroup\$

I have created a tab Accordion. at present it works fine. it should:

  1. open and close on click ( shuffling )
  2. on click of a tab other tabs should be closed if open already.

for this, I used 2 userEffect hook and a function to handle my click event. it is utilizing the provider from context as well. I am little bit confused about my approach considering performace.

please some one review the code and give the advice if I am wrong?

here is my code and demo:

  import React, {
  createContext,
  ReactNode,
  useContext,
  useEffect,
  useState
} from "react";
import "./styles.css";

export interface ContextProps {
  isExpand: number | null;
  childHandler: (v: number) => void;
}

export const initialContextProps: ContextProps = {
  isExpand: null,
  childHandler(v: number) {}
};

export const parentContext = createContext<ContextProps>(initialContextProps);
const { Provider } = parentContext;

function Children({ label, si }: { label: string; si: number }): JSX.Element {
  const { isExpand, childHandler } = useContext(parentContext);
  const [classProps, setClassProps] = useState(false);
  useEffect(
    function () {
      if (si === isExpand) {
        setClassProps(true);
      }
    },
    [setClassProps, si, isExpand]
  );

  useEffect(
    function () {
      if (si !== isExpand) {
        setClassProps(false);
      }
    },
    [setClassProps, si, isExpand]
  );

  function setClass(index: number) {
    setClassProps((prev) => !prev);
    childHandler(index);
  }

  return (
    <li onClick={() => setClass(si)} className={classProps ? "expand" : ""}>
      {label} :: {si} || {isExpand} -- {classProps}
      {classProps && <div>Content</div>}
    </li>
  );
}

function Parent({ children }: { children: ReactNode }): JSX.Element {
  const [isExpand, setIsExpand] = useState<number | null>(0);
  function childHandler(si: number) {
    setIsExpand(si);
  }
  return (
    <Provider value={{ isExpand, childHandler }}>
      <ul>{children} </ul>
    </Provider>
  );
}

export default function App() {
  return (
    <div className="App">
      <Parent>
        <Children key={1} label="Title-1" si={0} />
        <Children key={2} label="Title-2" si={1} />
        <Children key={3} label="Title-3" si={2} />
        <Children key={4} label="Title-4" si={3} />
        <Children key={5} label="Title-5" si={4} />
      </Parent>
    </div>
  );
}

LIVE-DEMO

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Nice job having it worked! However a few improvements can be done:

Comments about coding conventions:

  • Please name your variables, functions, parameters properly, so that it's easier for other people to review your code. For example, si and index refer to the same thing, you should keep the naming consistent throughout your work. Another example is, Parent, Children, childHandler sound confusing, please rename them into the actual thing you are making, like Accordion, AccodionItem, onOpen, etc.
  • You can upload your snippet onto github, so that reviewers can easily target at specific lines and point out the exact problem.

Problems about your code:

Basically your code is an overkill to what you wanna acheive.

  • Use React Context only on something that doesn't change often, because

All consumers that are descendants of a Provider will re-render whenever the Provider’s value prop changes.

  • You defined a boolean state under each child, which is unnecessary if you are already using Context. The expand state can be directly determined by si === isExpand
  • Two useEffect callbacks have the exact same dependencies, you can combine them into one, but again, you do not need them.

A better solution:

Think about the data structure of your components. It can be an array contains a number of objects, with each has properties id, label and isOpen. Make that your state and use .map to render them. In that way, React compares the props passed into your component and skip re-rendering if the props remain the same, so it's more performant in that way.

\$\endgroup\$
2
  • \$\begingroup\$ can you update my code as your wish? and post it? \$\endgroup\$
    – 3gwebtrain
    Jan 13, 2023 at 11:50
  • \$\begingroup\$ Another suggestion - you should always put into your efforts first then ask for help. I can help make edits to your update. If you need a bit of hint - you can search for to-do list implementation in React on internet, it incorporates the same idea as I mentioned in the solution \$\endgroup\$ Jan 13, 2023 at 13:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.