Ticket-88 #384
Ticket-88 #384
Conversation
|
@divitcr7 it looks like there are changes that revert to an older version of the study landing page - is this intended? |
|
@divitcr7 also could you please update the PR title to something more descriptive, thanks! |
|
This ticket details was changed initially after the task was done, as per the ticket we were first supposed to create a study banner, but as per the discussion only the notifications banner on the top right of the screen was supposed to be added , tht stated the no. of points |
| import Markdown from 'react-markdown' | ||
| import { useLearningPathStudies } from './learner/studies'; | ||
| import { CompactStudyCard } from '../components/study/compact-study-card'; | ||
| import { notifications } from '@mantine/notifications'; | ||
|
|
||
| let notificationShownThisSession = false; |
There was a problem hiding this comment.
I dont think we should be doing it this way - we don't want to be modifying a variable outside of the component state.
|
|
||
| return () => clearTimeout(timer); | ||
| } | ||
| }, [study.totalPoints]); |
There was a problem hiding this comment.
what happens if you remove study.totalPoints from the dependency array? ideally this should just run once when the component renders initially right?
There was a problem hiding this comment.
yes but, when i did that for some weird reason, the notifcations got displayed twice, one above another like this:
There was a problem hiding this comment.
this could be just a side effect of react rendering components twice in dev mode
| // Reset the flag when the component unmounts | ||
| useEffect(() => { | ||
| return () => { | ||
| notificationShownThisSession = false; |
There was a problem hiding this comment.
remove this RE: above comment
|
@divitcr7 did you confirm if it was just the react strictmode/local dev server that was causing the double render? i think we should remove the |
|
i’ll jump in here since i think i know the cause, it’s likely because react
triggers hooks twice in dev mode. search for “double useEffect” should
pull up docs
…On July 26, 2024 at 10:33:08, Chris Bendel ***@***.***) wrote:
@divitcr7 <https://github.com/divitcr7> did you confirm if it was just
the react strictmode/local dev server that was causing the double render? i
think we should remove the useState for notificationShown if possible...
should be unneeded if thats what it is? cc @Coder-Srinivas
<https://github.com/Coder-Srinivas> - curious what you think
—
Reply to this email directly, view it on GitHub
<#384 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAATNTRO3R4A432YKWRTI4DZOJT3JAVCNFSM6AAAAABK4XN2POVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJTGAYTAMJQGU>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
added notification banner after completion of study