-
Notifications
You must be signed in to change notification settings - Fork 41
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
feature: Warn user if they leave the /editor page with unsaved changes #109 #139
Changes from 8 commits
c4e5b60
3dfb23d
c54b41d
ce6682d
870ed44
3625d3f
eb90f9d
44d06ce
979698c
9b1b530
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,6 +46,7 @@ export const CodeEditor = ({ | |
onDtsChange?: (dts: string) => void | ||
initialCode: string | ||
readOnly?: boolean | ||
manualEditsJson?: string | ||
isStreaming?: boolean | ||
manualEditsFileContent: string | ||
showImportAndFormatButtons?: boolean | ||
|
@@ -86,7 +87,6 @@ export const CodeEditor = ({ | |
Object.entries(files).forEach(([filename, content]) => { | ||
fsMap.set(filename, content) | ||
}) | ||
|
||
createDefaultMapFromCDN( | ||
{ target: ts.ScriptTarget.ES2022 }, | ||
"5.6.3", | ||
|
@@ -128,9 +128,13 @@ export const CodeEditor = ({ | |
.replace(registryPrefixes[2], "") | ||
const packageName = fullPackageName.split("/")[0].replace(/\./, "/") | ||
const pathInPackage = fullPackageName.split("/").slice(1).join("/") | ||
const jsdelivrPath = `${packageName}${pathInPackage ? `/${pathInPackage}` : ""}` | ||
const jsdelivrPath = `${packageName}${ | ||
pathInPackage ? `/${pathInPackage}` : "" | ||
}` | ||
return fetch( | ||
`${apiUrl}/snippets/download?jsdelivr_resolve=${input.includes("/resolve/")}&jsdelivr_path=${encodeURIComponent(jsdelivrPath)}`, | ||
`${apiUrl}/snippets/download?jsdelivr_resolve=${input.includes( | ||
"/resolve/", | ||
)}&jsdelivr_path=${encodeURIComponent(jsdelivrPath)}`, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. revert unnecessary change here |
||
) | ||
} | ||
return fetch(input, init) | ||
|
@@ -263,7 +267,7 @@ export const CodeEditor = ({ | |
for (let pos = from; pos < to; ) { | ||
const line = view.state.doc.lineAt(pos) | ||
const lineText = line.text | ||
const matches = lineText.matchAll(/@tsci\/[\w\-.]+/g) | ||
const matches = lineText.matchAll(/@tsci\/[\w.]+/g) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this would introduce a bug, revert There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am sorry for all this trouble i am thinking to shut this pr and create a cleaner pr that is easier to manage for you and ofcourse myself There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes that's always ok! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and no problem- my messages are short because i often type them from my phone, not because i'm angry at you 😁 |
||
for (const match of matches) { | ||
if (match.index !== undefined) { | ||
const start = line.from + match.index | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
import { useEffect } from "react" | ||
export default function useWarnUserForUnsavedChanges({ | ||
hasUnsavedChanges, | ||
}: { | ||
hasUnsavedChanges: boolean | ||
}) { | ||
useEffect(() => { | ||
const handleBeforeUnload = (event: BeforeUnloadEvent) => { | ||
if (hasUnsavedChanges) { | ||
event.preventDefault() | ||
event.returnValue = "" // Shows the confirmation dialog on reload or close if there are unsaved changes | ||
} | ||
} | ||
|
||
const handlePopState = (event: PopStateEvent) => { | ||
if (hasUnsavedChanges) { | ||
const userConfirmed = window.confirm( | ||
"Are you sure you want to go back? Unsaved changes may be lost.", | ||
) | ||
if (!userConfirmed) { | ||
// Push the state again to stay on the same page | ||
window.history.pushState(null, "", window.location.href) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we have to push state to stay on the same page? Can we just not do anything and block the action? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. state is necessary here this solution works by storing the current url in the state and if user clicks on cancel instead of sending it to previous page it send it to the current page There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What would happen if you removed the pushState? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if i understand your code correctly, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It basically stores the current url (editor page) and only allows when cancelled. Is there a problem with storing the data in the state? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe to add this functionality we will need to have states involved. However, if you have better method in mind I can make it . There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's remove the pushStates, if we only work for reloading that's OK for now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as you wish but i will just comment out that part just in case you want to update There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's just remove it, i'd rather not have ghost code, people can come back and look at this PR. Also make sure to revert the changes to files where things were accidentally changed. I'll leave some comments that indicate where that happened. There should be no changes to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok will do |
||
} | ||
} | ||
} | ||
|
||
// Attach event listeners | ||
window.addEventListener("beforeunload", handleBeforeUnload) | ||
window.addEventListener("popstate", handlePopState) | ||
|
||
// Push the initial state only once, when component mounts | ||
if (hasUnsavedChanges) { | ||
window.history.pushState(null, "", window.location.href) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we need to push state here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we push the states two times:
|
||
} | ||
|
||
// Cleanup event listeners on component unmount | ||
return () => { | ||
window.removeEventListener("beforeunload", handleBeforeUnload) | ||
window.removeEventListener("popstate", handlePopState) | ||
} | ||
}, [hasUnsavedChanges]) | ||
} |
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.
revert unnecessary change here