zakodium-oss / react-science

React components and tools to build scientific applications.
https://react-science.pages.dev
MIT License
3 stars 6 forks source link

Remove the useEffect #671

Closed lpatiny closed 6 months ago

lpatiny commented 6 months ago

https://github.com/zakodium-oss/react-science/blob/main/src/components/header/PanelHeader.tsx#L47C1-L57C24

targos commented 6 months ago

@wadjih-bencheikh18 Do you remember this? How did you end up using an effect to set the paragraph contents?

wadjih-bencheikh18 commented 6 months ago

I tried now to use

const counterLabel = useMemo(() => {
    if (current !== undefined && total !== undefined) {
      return `${current} / ${total}`;
    }
    if (current !== undefined) {
      return `[ ${current} ]`;
    }
    if (total !== undefined) {
      return `[ ${total} ]`;
    }
    return '';
  }, [current, total]);

And it works fine I don't remember why I used effect

I even checked the old PR to understand why https://github.com/zakodium-oss/react-science/pull/523 / https://github.com/zakodium-oss/react-science/pull/523/commits/d68782cd2c711b10c74b9ba65ba46a2892bbb375

hamed-musallam commented 6 months ago

Why are you memoizing the counter label? there is no need for this, you can create a function outside the component to format the counter label

function formatCounterLabel(current,total) {
  if (current !== undefined && total !== undefined) {
      return `${current} / ${total}`;
    }
    if (current !== undefined) {
      return `[ ${current} ]`;
    }
    if (total !== undefined) {
      return `[ ${total} ]`;
    }
    return '';
}

and call the function here https://github.com/zakodium-oss/react-science/blob/fcb4a8eb6bd37d0c1f806140c227f7b1c5bfbbb0/src/components/header/PanelHeader.tsx#L62C1-L63C1