-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fix : #216 Mark as complete button UI sync #222
Conversation
src/components/Sidebar.tsx
Outdated
@@ -58,9 +59,19 @@ export function Sidebar({ | |||
const pathArray = findPathToContent(fullCourseContent, contentId); | |||
if (pathArray) { | |||
const path = `/courses/${courseId}/${pathArray.join('/')}`; | |||
console.log(`Path is this ${path}`); |
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.
could you remove this log
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.
In the commit named removed debug statements I have removed these logs
isCompleted? : boolean | ||
} | ||
|
||
export const markAsCompleteAtom = atom<markAsCompleteParams>({ |
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.
This approach seems slightly ad-hoc-ish
You're using a single atom for all checkboxes
You probably need an atom family here
@hkirat So you mean for every module of the course there should be an atom in the atom-family, atom's value should change on triggering checkbox or markasComplete button ? |
yes! each atom in the atomfamily should be identified using the courseId |
@hkirat Got your point, implementing it |
@hkirat This time I changed the approach and tried using Family . ] Whenever from Sidebar or ContentRenderer someone hits marksAsComplete or checks the box, firstly I will make the backend request and then update the state for that specific content id Is this time my approach fine ? I am stuck in a situation, in atomFamily using atomSelectors , nextJS is allowing me to use server function in client component. I need to getFullCourseContent function to get the course data and convert into this format [{ id : 2, ] and store in state but next is not letting me allow it Any suggestions ? |
@hkirat or should I use a single atom for whole course and when ContentRendere component is mounted I should initialise its data using useEffect and use in Sidebar and other places. Whenever a user will change course, ContentRenderer component will refresh the state according to that course |
We need the data to be hydrated from the server for SEO |
yeah you cant set state on the server |
@hkirat Got it... what about my second approach [{ id : 2, whenever changes happen from SideBar or ContentRendererClient, it will rerender itself. You were saying probably we need atomFalimy to keep state of all courses but I think whenever Sidebar will be mounted it will automatically initialise atom values according to that course only Share your thoughts on this approach |
Hi @hkirat , to solve this issue I observed that whenever a user is changing content then the sidebar reload data from backend and the checkbox is checked according to completed or not but the problem is only when a user is on the same page and tried to check or click on markascompleted then it could not see the changes in the sidebar and the main content screen accordingly. If sidebar again reloads the data then it will show the correct checked status.
My Approach
I created a atom for this, if any user is changing checkbox then this atom will hold its value and will reflect on the right side and vice versa. As user changes content page, sidebar again gets data from backend and the role of atom is complete.
I am just a beginner, please suggest for the improvement required
I have attached the video after solving the issue
Untitled.video.-.Made.with.Clipchamp.mp4