-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: prompt to save generator on exit #293
Conversation
useEffect(() => { | ||
window.studio.app.changeRoute(location.pathname) | ||
}, [location]) |
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'm keeping track of the routes on the electron side so we know which route the close
event in the main window came from.
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.
Reviewing from the functionality standpoint, works great 🙌
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.
LGTM. Thought the copy could be improved for this specific case. Maybe something like this will work in both cases:
You have unsaved changes in the generator which will be lost if you proceed
Although I see that it's not ideal either
what about the same text plus It should work both for leaving the page and leaving the application! |
5c7399c
@Llandy3d I liked your suggestion. Just pushed a change with it! |
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.
🙌
Description
This PR triggers the dialog to save the generator (if unsaved changes are present) when closing the main window.
How to Test
Checklist
npm run lint
) and all checks pass.npm test
) and all tests pass.Screenshots (if appropriate):
Screen.Recording.2024-10-30.at.10.18.57.AM.mov
Related PR(s)/Issue(s)
Resolves #239