refactoring review-points into rankStep

app/components/ranking.jsx

@@ -24,6 +24,7 @@ export default function Ranking(props) {
         onDone && onDone({ valid: false, value: '' })
       } else {
         setResponse(defaultValue)
+        onDone && onDone({ valid: true, value: defaultValue })
       }
     }
   }, [defaultValue])

app/components/review-point.jsx

@@ -14,92 +14,65 @@ import { H, Level } from 'react-accessible-headings'
 
 function ReviewPoint(props) {
   const { point = {}, leftPointList = [], rightPointList = [], rank = '', onDone = () => {}, ...otherProps } = props
-  const [isRead, setIsRead] = useState(false)
-  const [isOpened, setIsOpened] = useState(false)
-  const [isRanked, setIsRanked] = useState(rank !== '')
-  const [isRankActive, setIsRankActive] = useState(false)
+  const [isRanked, setIsRanked] = useState(false)
 
-  const classes = useStylesFromThemeFunction()
-
-  useEffect(() => {
-    if (isOpened) {
-      setIsRead(true)
-    }
-  }, [isOpened])
-
-  useEffect(() => {
-    if (isRead) {
-      setIsRankActive(true)
-    } else {
-      setIsRankActive(false)
-    }
-  }, [isRead])
+  // disabled: can't select yet, collapesed, has not been read
+  // open: point is open and visible, can select a ranking, has been read
+  // enabled: point is collapsed, but you can still select a ranking, has been read
+  const [vState, setVState] = useState(rank ? 'enabled' : 'disabled')
 
-  useEffect(() => {
-    if (isRanked) {
-      setIsRead(true)
-    }
-  }, [isRanked])
-
-  useEffect(() => {
-    if (!isRanked && !isOpened) {
-      setIsRead(false)
-    }
-  }, [isRanked, isOpened])
+  const classes = useStylesFromThemeFunction()
 
-  const handleRankingDone = selectedRank => {
-    setIsOpened(false)
-    setIsRanked(selectedRank !== '')
-    if (rank !== selectedRank) {
-      onDone(selectedRank)
+  const handleRankingDone = ({ valid, value }) => {
+    setVState('enabled')
+    setIsRanked(valid)
+    if (!(isRanked && rank === value)) {
+      onDone({ valid, value })
     }
   }
 
   useEffect(() => {
-    if (rank) {
-      setIsRanked(true)
-    } else {
-      setIsRanked(false)
-    }
+    if (rank) setVState('enabled')
+    if (isRanked) setIsRanked(false) // not ranked until validated from child
   }, [rank])
 
   return (
     <div className={cx(classes.borderStyle)} {...otherProps}>
       <div className={cx(classes.contentContainer)}>
         <div className={classes.informationGrid}>
           <div className={classes.informationColumn}>
-            <span className={isRead ? classes.statusBadgeComplete : classes.statusBadge}>
-              {isRead ? 'Read' : 'Unread'}
+            <span className={vState !== 'disabled' ? classes.statusBadgeComplete : classes.statusBadge}>
+              {vState !== 'disabled' ? 'Read' : 'Unread'}
             </span>
             {point.subject && <H className={cx(classes.subjectStyle)}>{point.subject}</H>}
             {point.description && <p className={cx(classes.descriptionStyle)}>{point.description}</p>}
           </div>
           <div className={classes.rankingColumn}>
             <Ranking
               className={classes.ranking}
-              disabled={!isRankActive}
+              disabled={vState === 'disabled'}
               defaultValue={rank}
               onDone={handleRankingDone}
             />
           </div>
         </div>
         <div className={classes.SvgContainer}>
-          {isOpened ? (
-            <TextButton onClick={() => setIsOpened(false)} title="close" tabIndex={0}>
+          {vState === 'open' ? (
+            <TextButton onClick={() => setVState('enabled')} title="close" tabIndex={0}>
               <span className={classes.chevronButton}>
                 <SvgChevronUp />
               </span>
             </TextButton>
           ) : (
-            <TextButton onClick={() => setIsOpened(true)} title="open" tabIndex={0}>
+            <TextButton onClick={() => setVState('open')} title="open" tabIndex={0}>
               <span className={classes.chevronButton}>
                 <SvgChevronDown />
               </span>
             </TextButton>
           )}
         </div>
       </div>
-      {isRead && isOpened && (leftPointList.length > 0 || rightPointList.length > 0) && (
+      {vState === 'open' && (leftPointList.length > 0 || rightPointList.length > 0) && (
         <div className={classes.showDualPointListContainer}>
           <Level>
             <ShowDualPointList

app/components/steps/__tests__/rerank.js

@@ -34,7 +34,7 @@ jest.mock('react-accessible-headings', () => {
 const data = {}
 describe('test derivePoinMostsLeastsRankList', () => {
   test('no data yields no chanage', () => {
-    const reviewPoints = derivePointMostsLeastsRankList(data)
+    const { reviewPoints } = derivePointMostsLeastsRankList(data)
     expect(reviewPoints).toBe(undefined)
   })
   test('partial data yields no change', () => {
@@ -43,14 +43,14 @@ describe('test derivePoinMostsLeastsRankList', () => {
       { point: { _id: '2', subject: '2', description: '2' } },
       { point: { _id: '3', subject: '3', description: '3' } },
     ]
-    const reviewPoints = derivePointMostsLeastsRankList(data)
+    const { reviewPoints } = derivePointMostsLeastsRankList(data)
     expect(reviewPoints).toMatchObject(data.reducedPointList)
-    const newReviewPoints = derivePointMostsLeastsRankList(data)
+    const newReviewPoints = derivePointMostsLeastsRankList(data).reviewPoints
     expect(newReviewPoints).toBe(reviewPoints)
   })
   test("ref doesn't change if data doesn't change", () => {
-    const reviewPoints = derivePointMostsLeastsRankList(data)
-    const newReviewPoints = derivePointMostsLeastsRankList(data)
+    const { reviewPoints } = derivePointMostsLeastsRankList(data)
+    const newReviewPoints = derivePointMostsLeastsRankList(data).reviewPoints
     expect(newReviewPoints).toBe(reviewPoints)
   })
   test('ref does change if a point changes', () => {
@@ -62,19 +62,17 @@ describe('test derivePoinMostsLeastsRankList', () => {
     expect(newReviewPoints).toEqual(reviewPoints)
   })
   test('works with top ranked whys', () => {
-    data.topWhyByParentId = {
+    data.topWhyById = {
       4: { _id: '4', subject: '1.4 top most', description: '1.4 top most', parentId: '1', category: 'most' },
       5: { _id: '5', subject: '1.5 top least', description: '1.5 top least', parentId: '1', category: 'least' },
       6: { _id: '6', subject: '2.6 top most', description: '2.6 top most', parentId: '2', category: 'most' },
       7: { _id: '7', subject: '2.7 top least', description: '2.7 top least', parentId: '2', category: 'least' },
     }
-    const reviewPoints = derivePointMostsLeastsRankList(data)
+    const { reviewPoints } = derivePointMostsLeastsRankList(data)
     const calculatedReviewPoints = data.reducedPointList.map(({ point }) => {
       // if there are no mosts or leasts, the entry should not be present
-      const mosts = Object.values(data.topWhyByParentId).filter(
-        why => why.parentId === point._id && why.category === 'most'
-      )
-      const leasts = Object.values(data.topWhyByParentId).filter(
+      const mosts = Object.values(data.topWhyById).filter(why => why.parentId === point._id && why.category === 'most')
+      const leasts = Object.values(data.topWhyById).filter(
         why => why.parentId === point._id && why.category === 'least'
       )
       const result = { point }
@@ -85,24 +83,24 @@ describe('test derivePoinMostsLeastsRankList', () => {
     expect(reviewPoints).toMatchObject(calculatedReviewPoints)
   })
   test("ref doesn't change if point and top whys doesn't change", () => {
-    const reviewPoints = derivePointMostsLeastsRankList(data)
-    const newReviewPoints = derivePointMostsLeastsRankList(data)
+    const { reviewPoints } = derivePointMostsLeastsRankList(data)
+    const newReviewPoints = derivePointMostsLeastsRankList(data).reviewPoints
     expect(newReviewPoints).toBe(reviewPoints)
   })
   test('ref does change if a point changes when there are ranks', () => {
-    const reviewPoints = derivePointMostsLeastsRankList(data)
+    const { reviewPoints } = derivePointMostsLeastsRankList(data)
     data.reducedPointList[1] = { point: { ...data.reducedPointList[1].point } }
     data.reducedPointList = [...data.reducedPointList]
-    const newReviewPoints = derivePointMostsLeastsRankList(data)
+    const newReviewPoints = derivePointMostsLeastsRankList(data).reviewPoints
     expect(newReviewPoints).not.toBe(reviewPoints)
     expect(newReviewPoints).toEqual(reviewPoints)
   })
   test('ref does change if a why is changed', () => {
-    const reviewPoints = derivePointMostsLeastsRankList(data)
+    const { reviewPoints } = derivePointMostsLeastsRankList(data)
     const reviewPoint0 = reviewPoints[0]
-    data.topWhyByParentId['6'] = { ...data.topWhyByParentId['6'] }
-    data.topWhyByParentId = { ...data.topWhyByParentId }
-    const newReviewPoints = derivePointMostsLeastsRankList(data)
+    data.topWhyById['6'] = { ...data.topWhyById['6'] }
+    data.topWhyById = { ...data.topWhyById }
+    const newReviewPoints = derivePointMostsLeastsRankList(data).reviewPoints
     expect(newReviewPoints).not.toBe(reviewPoints)
     expect(newReviewPoints).toEqual(reviewPoints)
     expect(newReviewPoints[0]).toEqual(reviewPoint0)
@@ -113,13 +111,11 @@ describe('test derivePoinMostsLeastsRankList', () => {
       2: { _id: '9', category: 'neutral', parentId: '2', stage: 'post' },
       3: { _id: '10', category: 'least', parentId: '3', stage: 'post' },
     }
-    const reviewPoints = derivePointMostsLeastsRankList(data)
+    const { reviewPoints } = derivePointMostsLeastsRankList(data)
     const calculatedReviewPoints = data.reducedPointList.map(({ point }) => {
       // if there are no mosts or leasts, the entry should not be present
-      const mosts = Object.values(data.topWhyByParentId).filter(
-        why => why.parentId === point._id && why.category === 'most'
-      )
-      const leasts = Object.values(data.topWhyByParentId).filter(
+      const mosts = Object.values(data.topWhyById).filter(why => why.parentId === point._id && why.category === 'most')
+      const leasts = Object.values(data.topWhyById).filter(
         why => why.parentId === point._id && why.category === 'least'
       )
       const result = { point }

app/components/steps/rerank.js

@@ -83,21 +83,19 @@ export function Rerank(props) {
   // call onDone to notify parent of new values, and add the delta prop to pass what's changed to the parent
   const handleReviewPoint = (point, result) => {
     const rankString = result.value
-    let rank
-    let percentDone
     // the above vars are needed when calling onDone which must be done outside the set function
     setRankByParentId(rankByParentId => {
-      // doin this within the set function because handleReviewPoint could get called multiple time before the next rerender which updates the state value returned by useState
+      // doing this within the set function because handleReviewPoint could get called multiple time before the next rerender which updates the state value returned by useState
+      let rank
+      let percentDone
       if (rankByParentId[point._id]) {
         if (rankByParentId[point._id].category !== rankStringToCategory[rankString]) {
           rank = { ...rankByParentId[point._id], category: rankStringToCategory[rankString] }
           rankByParentId[point._id] = rank // mutate the state don't call the set function
           percentDone = Object.keys(rankByParentId).length / reviewPoints.length
-          return rankByParentId
         } else {
           percentDone = Object.keys(rankByParentId).length / reviewPoints.length
           rank = rankByParentId[point._id]
-          return rankByParentId // nothing has changed, so abort the setter and don't cause a rerender
         }
       } else {
         rank = {
@@ -110,10 +108,10 @@ export function Rerank(props) {
         }
         rankByParentId[point._id] = rank
         percentDone = Object.keys(rankByParentId).length / reviewPoints.length
-        return rankByParentId
       }
+      if (rank) setTimeout(() => onDone({ valid: percentDone === 1, value: percentDone, delta: rank })) // don't call onDone from within a setter - because onDone's may call other react hooks and react causes errors
+      return rankByParentId // about the setter
     })
-    if (rank) onDone({ valid: percentDone === 1, value: percentDone, delta: rank })
   }
 
   if (!reviewPoints) return null // nothing ready yet
@@ -168,7 +166,7 @@ const useStylesFromThemeFunction = createUseStyles(theme => ({
 // to make is possible to test with jest, this is exported
 export function derivePointMostsLeastsRankList(data) {
   const local = useRef({ reviewPointsById: {} }).current
-  const { reducedPointList, postRankByParentId, topWhyByParentId } = data
+  const { reducedPointList, postRankByParentId, topWhyById } = data
   let updated = false
 
   const { reviewPointsById } = local
@@ -184,11 +182,11 @@ export function derivePointMostsLeastsRankList(data) {
     }
     local.reducedPointList = reducedPointList
   }
-  if (local.topWhyByParentId !== topWhyByParentId) {
-    local.topWhyByParentId = topWhyByParentId
+  if (local.topWhyById !== topWhyById) {
+    local.topWhyById = topWhyById
     let index
     const categoiesToUpdateByParentId = {}
-    for (const whyPoint of Object.values(topWhyByParentId)) {
+    for (const whyPoint of Object.values(topWhyById)) {
       if (!reviewPointsById[whyPoint.parentId]) continue // parent not in pointList
       const category = whyPoint.category
       if ((index = reviewPointsById[whyPoint.parentId][category + 's']?.findIndex(w => w._id === whyPoint._id)) >= 0) {
@@ -225,6 +223,5 @@ export function derivePointMostsLeastsRankList(data) {
     local.postRankByParentId = postRankByParentId
   }
   if (updated) local.reviewPoints = Object.values(local.reviewPointsById)
-  console.info('derive', JSON.stringify(local.reviewPoints, null, 2))
   return { reviewPoints: local.reviewPoints }
 }