-
-
Notifications
You must be signed in to change notification settings - Fork 30
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
Save the code and add checkpoint so the users can start from where they left off #102
Conversation
import { getCheckPoint } from '@/lib/progressSaving' | ||
|
||
|
||
export default function CheckpointRedirect() { |
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.
Same question as in the previous PR: is there any reason to implement the redirect in a component, instead of a plan function?
I can't see much reason for using useEffect() here - @JeelRajodiya can you please advise?
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 know if there is any other way to redirect without useEffect in this case.
I think @pavanydg has separated the code in a component because it requires the client side hook useEffect(), while our home page is a server side component. (When to use Server and Client Components?)
@@ -85,7 +98,9 @@ export default function CodeEditor({ | |||
theme={colorMode === "light" ? "light" : "my-theme"} | |||
value={codeString} | |||
height={"100%"} | |||
onChange={(codeString) => setCodeString(codeString ? codeString : "")} | |||
onChange={(codeString) => | |||
setCodeString(codeString ? codeString : "") |
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.
Can you please use either ||
or ??
operator here?
@@ -83,6 +85,7 @@ export default function NavBarMenu() { | |||
duration: 3000, | |||
isClosable: true, | |||
}); | |||
window.location.reload(); |
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.
Can we please avoid reloading the whole window?
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.
import { getCheckPoint } from '@/lib/progressSaving' | ||
|
||
|
||
export default function CheckpointRedirect() { |
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 know if there is any other way to redirect without useEffect in this case.
I think @pavanydg has separated the code in a component because it requires the client side hook useEffect(), while our home page is a server side component. (When to use Server and Client Components?)
@pavanydg Great work! the feature is working as expected, we just need some refactoring in the code. Please see my comment above. |
@JeelRajodiya pl review |
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.
It becomes difficult to follow what is codeString / codeData / codeStore etc. I suggest the following naming changes:
- CodeStore -> UserSolutionStore
- useCodeStore -> useUserSolutionStore
- CodeData -> UserSolutionsByLesson
- codeString -> displayedSolution
- persistCode -> saveUserSolutionForLesson
- getCode -> getSavedUserSolutionByLesson
WDYT?
@erosb yeah sure. Thanks for the suggestions. |
@erosb i have a doutbt. codeString is passed in other components as prop like editornoutput.tsx, client-functions.ts. So if i change in codeeditor.tsx i have to change there also. Should i still do it? i have done other changes as requested. |
@JeelRajodiya @erosb pl review. |
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.
@pavanydg I encountered one conflicting condition, When you are on any lesson and click on the JSON Schema icon from the nav bar (which is supposed to lead you to the landing page), but when the landing page opens, it immediately redirects you to the lesson page. please view the below video.
input.mp4
I am not sure quite if this will work or not, but you can try to resolve this using a global sate which can keep track where the user lands to the landing page from a content page.
@JeelRajodiya pl review. This fixes the above problem. Let me know if still we need to go through the global state approach for the checkpoint. |
@JeelRajodiya any changes needed? |
@pavanydg I was just reviewing your code, and realized that instead of directly redirecting the users from the home page It would be better if we could give the users a button such as "Continue" in the home page. It would give them more autonomy in navigation, and would resolve the above error as well. The continue button will be only visible when the users has some checkpoints saved, it won't be visible to new users who do not have any checkpoint saved. Please take the reference of the below design to add the continue button I apologies for the change and the delay. |
Yeah sure. Will do it. |
@pavanydg thanks, It would also helpful if you know any better way to place and style this button. in case, you have seen a similar design somewhere you can share it. Otherwise you can continue working on the design I suggested. |
@JeelRajodiya pl review |
@JeelRajodiya need any changes? |
We finally have the code saving feature in the tour. Great work @pavanydg |
@JeelRajodiya @erosb thank you for your guidance. Looking forward to make more meaningful contributions. |
What kind of change does this PR introduce?
Saves the code of each lesson if user has written any code in it.
Also adds a checkpoint from where users can resume their learning when open the website again.
Issue Number:
Screenshots/videos:
Screencast.from.10-17-2024.11.25.16.PM.webm
If relevant, did you update the documentation?
Summary
Does this PR introduce a breaking change?