Skip to content
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(#379): confirm exit from playground #515

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

km4
Copy link
Contributor

@km4 km4 commented Oct 13, 2020

Closes #379

@mhagmajer
Copy link
Contributor

@czerwinskilukasz1 looks like @km4 is taking care of #379 that you have raised :)

@km4 km4 requested a review from YonatanKra October 18, 2020 16:04
Copy link
Contributor

@mhagmajer mhagmajer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR @km4! Please apply Yonatan's suggestion and we can go with this 👍

@km4
Copy link
Contributor Author

km4 commented Oct 19, 2020

@mhagmajer I see the light in the tunnel :P

Copy link
Contributor

@mhagmajer mhagmajer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this @km4 ! Just a few nits below

src/playground/public/assets/js/app.ts Outdated Show resolved Hide resolved
src/playground/public/assets/js/app.ts Outdated Show resolved Hide resolved
src/playground/public/assets/js/app.ts Show resolved Hide resolved
src/playground/public/assets/js/app.ts Outdated Show resolved Hide resolved
@km4 km4 requested a review from mhagmajer October 30, 2020 19:19
@km4
Copy link
Contributor Author

km4 commented Nov 1, 2020

@mhagmajer some update, check now and add feedback

Copy link
Contributor

@mhagmajer mhagmajer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting some finishing touches :)

if (dirty) return;
playgroundIsDirty(!dirty);
dirty = true;
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that now when we have isDirty check, we can just attach onbeforeunload once for all without the need for the extra playgroundIsDirty function.

Suggested change
});
});
window.onbeforeunload = function (e: any) {
if (isDirty) {
e.preventDefault();
e.returnValue = '';
}
};

@@ -81,6 +87,7 @@ async function executeAskScript(
const json = await response.json();
if (response.status == 200) {
showSuccessfulResponse(json.result);
playgroundIsDirty(false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
playgroundIsDirty(false);
isDirty = false;

Copy link
Contributor Author

@km4 km4 Nov 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mhagmajer
what about this isDirty flag? Now is undefined variable for this scope. If I push dirty params to all functions eg.

async function executeAskScript(
  askScriptCode: string,
  askScriptServerUrl: string,
  isDirty: boolean
) {

executeAskScriptFromEditor(editor, askScriptServerUrl, isDirty); the flag dosnt update in the top.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ask for confirmation when user triggers going Back or Forward on Playground page
3 participants