-
Notifications
You must be signed in to change notification settings - Fork 430
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
Await for confirm method results #525
Conversation
Tests are failing because |
@@ -113,7 +113,7 @@ export class FormSubmission { | |||
const { initialized, requesting } = FormSubmissionState | |||
|
|||
if (this.needsConfirmation) { | |||
const answer = FormSubmission.confirmMethod(this.confirmationMessage!, this.formElement) | |||
const answer = await FormSubmission.confirmMethod(this.confirmationMessage!, this.formElement) |
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.
Switching to an assumption that FormSubmission.confirmMethod
is a promise, what happens when a user-provided Promise set by Turbo.setConfirmMethod
throws an exception?
Before the await
, the async start()
(the method that wraps these lines) would have to deal with the exception.
After adding the await
, is that exception swallowed?
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.
Throwing an error in the promise raises the Uncaught (in promise)
error in the start()
method of FormSubmission:
turbo/src/core/drive/form_submission.ts
Line 122 in 201f8b3
return this.fetchRequest.perform() |
It doesn't swallow the exception.
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.
That's good. In that case, should the default function also be translated into a Promise?
turbo/src/core/drive/form_submission.ts
Lines 53 to 55 in 201f8b3
static confirmMethod(message: string, element: HTMLFormElement):boolean { | |
return confirm(message) | |
} |
static confirmMethod(message: string, element: HTMLFormElement): Promise<boolean> {
return Promise.resolve(confirm(message))
}
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 didn't affect the default method, but we could.
Would we also want to change this to expect a Promise too?
export function setConfirmMethod(confirmMethod: (message: string, element: HTMLFormElement)=>Promise<boolean>) {
FormSubmission.confirmMethod = confirmMethod
}
Great work @excid3! Thank you! |
@kstratis I botched the rebase from main on accident. 😅 |
any update on this? @excid3 |
Is there anything actionable here? How can we move this forward? cc @dhh |
@dhh fixed this, so it's ready for review now. 👍 |
Tests that are failing are the same ones in |
@excid3 congrats on the merge! Does this mean that you're going to have a GoRails episode on implementing a custom confirm modal? 😇 |
@excid3 how would you go about i18n buttons rendered in |
@rapcal Lots of options. You could pass the text as extra data attributes on the element with the Or have Rails render the HTML for the dialog ahead of time like we do in the screencast linked here. #525 (comment) |
Thanks, Chris! |
The browser's
confirm
is synchronous and waits for a boolean. This is really convenient, but makes it painful to implement a custom confirm modal in Boostrap, Tailwind, etc.This PR adds
await
so a customconfirm
can return a promise that waits until the user confirms or cancels the operation.The overridden confirm method simply needs to return a Promise that resolves to
true
or `false.Closes #522
Example of it in action:
https://user-images.githubusercontent.com/67093/151271318-5034bbff-6b24-462f-89f7-8e6e3f8d0466.mp4