-
Notifications
You must be signed in to change notification settings - Fork 16
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
Reset hasLaunched after closing the instant launch dialog #609
Reset hasLaunched after closing the instant launch dialog #609
Conversation
…be reopened to launch again.
@@ -74,6 +74,8 @@ function InstantLaunchButtonWrapper(props) { | |||
window.open(`${getHost()}${ilUrl}`); | |||
setIlUrl(null); | |||
setOpen(false); | |||
// reset the value so after it closes you can launch a second by reopening | |||
setHasLaunched(false); |
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.
The main thing I'm not sure about here is: should I instead reset this in the onClick
function when the dialog is reopened, leaving it set to true
while the dialog is closed? Or does it not make any real difference, heh.
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 guess it doesn't seem to make a difference, but there's a lot of state and effects to juggle in this component, and it's a bit challenging to follow all the edge cases without actually interacting with it (either in dev mode or in storybook) 😅
But I think it makes more sense to me to reset it here.
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 looks good to me, but I'm not sure how to answer the question that you had. It might be best to wait for @psarando to look this over before we deploy it. 😁
@@ -74,6 +74,8 @@ function InstantLaunchButtonWrapper(props) { | |||
window.open(`${getHost()}${ilUrl}`); | |||
setIlUrl(null); | |||
setOpen(false); | |||
// reset the value so after it closes you can launch a second by reopening | |||
setHasLaunched(false); |
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 guess it doesn't seem to make a difference, but there's a lot of state and effects to juggle in this component, and it's a bit challenging to follow all the edge cases without actually interacting with it (either in dev mode or in storybook) 😅
But I think it makes more sense to me to reset it here.
Thanks for the reviews! If it breaks stuff to have it where I do I haven't hit it yet, anyway, so sounds like that's probably good enough for now, haha. |
this way it can be reopened to launch again.