-
Notifications
You must be signed in to change notification settings - Fork 481
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Legend responds to levels viewed #56357
Conversation
@@ -28,10 +30,26 @@ function SectionProgressV2({ | |||
} | |||
}, [unitData, isLoadingProgress, isRefreshingProgress, scriptId, sectionId]); | |||
|
|||
React.useEffect(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: At one point I wanted to move all this logic to the IconKey
component but given that would require passing all the unitData
to the child component, putting the logic here felt like a better choice.
const [isViewingLevelProgress, setIsViewingLevelProgress] = useState(false); | ||
|
||
React.useEffect(() => { | ||
setIsViewingLevelProgress(expandedLessonIds.length > 0); | ||
}, [expandedLessonIds]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this case calls for a useEffect
. There's no side effect here, it's just a very simple bit of logic to determine rendering within this component.
const [isViewingLevelProgress, setIsViewingLevelProgress] = useState(false); | |
React.useEffect(() => { | |
setIsViewingLevelProgress(expandedLessonIds.length > 0); | |
}, [expandedLessonIds]); | |
const isViewingLevelProgress = expandedLessonIds.length > 0; |
React.useEffect(() => { | ||
if (expandedLessonIds.length > 0) { | ||
const hasValidatedLevel = unitData.lessons.some( | ||
lesson => | ||
lesson.levels.some(level => level.isValidated) && | ||
expandedLessonIds.includes(lesson.id) | ||
); | ||
setIsViewingValidatedLevel(hasValidatedLevel); | ||
} else { | ||
setIsViewingValidatedLevel(false); | ||
} | ||
}, [expandedLessonIds, unitData]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job using functional array methods here! I think useMemo
is more appropriate for this case. Again, we're not causing any side effects such as calling an API, we're saving the results of a relatively expensive calculation. That's exactly what useMemo
is for.
React.useEffect(() => { | |
if (expandedLessonIds.length > 0) { | |
const hasValidatedLevel = unitData.lessons.some( | |
lesson => | |
lesson.levels.some(level => level.isValidated) && | |
expandedLessonIds.includes(lesson.id) | |
); | |
setIsViewingValidatedLevel(hasValidatedLevel); | |
} else { | |
setIsViewingValidatedLevel(false); | |
} | |
}, [expandedLessonIds, unitData]); | |
const isViewingValidatedLevel = React.useMemo(() => { | |
return unitData.lessons | |
.filter(lesson => expandedLessonIds.includes(lesson.id)) | |
.some(lesson => lesson.levels.some(level => level.isValidated)); | |
}, [expandedLessonIds, unitData]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks - this makes sense to me after reading the useMemo
documentation.
Modifies which icons appear depending on what lessons and levels are open. The video below shows this change working.
Screen.Recording.2024-02-12.at.12.28.44.PM.mov
The original figma shows what it should look like in these different states.
Links
Ticket.
Testing story
I tested it locally. I have another ticket to write up tests and other pieces of the icon key feature.
Deployment strategy
Follow-up work
Future work will address the spacing between the boxes in the icon key.
Privacy
Security
Caching
PR Checklist: