DEV Community

Discussion on: Thinking in React Reference

View post
Collapse
 
blnkspace profile image
AVI •

Hey given this is for beginners, please add a comment about your useEffect that selecting DOM elements and mutating their styles is totally against React; and that no one in production should ever need to directly get a DOM element if they're using React the way it's meant to be used.

Collapse
 
tlylt profile image
Liu Yongliang • • Edited

Hey Aviral, appreciate your candid reply.

I agree that my solution is not best practice. I guess when you say "using React the way it is meant to be used", it means this, straight from the React Doc:

React elements are immutable. Once you create an element, you can’t change its children or attributes. An element is like a single frame in a movie: it represents the UI at a certain point in time.

Hence, I have removed the useEffect that tries to manipulate DOM in a direct way. Instead of that, I will simply put the logic of toggling of the className="active" into the functional component that creates the span element. Would you say that is an improvement in terms of code quality?

From

function StepNumberBar({ total }) {
  return (
    <div className="stepNumberBar">
      {Array(total)
        .fill(null)
        .map((value, index) => (
          <span id={index} key={index}>
            {index}
          </span>
        ))}
    </div>
  );
}

To

function StepNumberBar({ total, current }) {
  return (
    <div className="stepNumberBar">
      {Array(total)
        .fill(null)
        .map((value, index) => (
          <span
            id={index}
            key={index}
            className={current === index ? "active" : "inactive"}
          >
            {index}
          </span>
        ))}
    </div>
  );
}

And the useEffect has been commented out

function ReferenceTable({ detail }) {
  const [currPage, updatePage] = useState(0);
  // useEffect(() => {
  //   var currP = document.querySelector("span[id='" + currPage + "']");
  //   var allSpan = document.querySelectorAll("span");
  //   allSpan.forEach((item) => {
  //     item.style.color = "black";
  //     item.className = item.className.replace(/active/g, "");
  //   });
  //   currP.style.color = "grey";
  //   currP.classList += ["active"];
  // }, [currPage]);
  return (
    <>
      <StepNumberBar total={detail.length} current={currPage} />
      <Explanation
        heading={detail[currPage].heading}
        content={detail[currPage].content}
      />
      <NavigationRow updatePage={updatePage} />
    </>
  );
}

I will amend the code in the article accordingly. Your further critique is appreciated!
Cheers,
Yong