-
Notifications
You must be signed in to change notification settings - Fork 3.1k
[#69641] Configure Turbo to use custom confirmation dialogs [Version 1] #21527
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
base: dev
Are you sure you want to change the base?
Conversation
frontend/src/app/shared/components/modals/form-control/check-box.component.ts
Fixed
Show fixed
Hide fixed
frontend/src/app/shared/components/modals/form-control/check-box.component.ts
Fixed
Show fixed
Hide fixed
frontend/src/app/shared/components/modals/form-control/text-field.component.ts
Fixed
Show fixed
Hide fixed
frontend/src/app/shared/components/modals/form-control/text-field.component.ts
Fixed
Show fixed
Hide fixed
2505b10 to
808d7c4
Compare
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.
Pull request overview
This PR configures Turbo to use custom confirmation dialogs instead of the browser's native confirm() dialog. It replaces the old deprecated setProgressBarDelay API with the new config-based approach, updates TypeScript type definitions to use the official @types/hotwired__turbo package, and implements a custom confirmation handler that integrates with OpenProject's existing ConfirmDialogService.
Changes:
- Configured Turbo to use a custom confirm dialog handler that integrates with OpenProject's ConfirmDialogService
- Updated type definitions to use official
@types/hotwired__turbotypes instead of custom definitions - Migrated from deprecated
setProgressBarDelay()to config-based API
Reviewed changes
Copilot reviewed 5 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/turbo/confirm.ts | New file implementing custom Turbo confirmation dialog using ConfirmDialogService |
| frontend/src/turbo/setup.ts | Configures Turbo with custom confirm handler and updates to use config API |
| frontend/src/typings/turbo.d.ts | Removed custom type definitions that are now provided by @types/hotwired__turbo |
| frontend/src/typings/shims.d.ts | Added Turbo config type definitions with forms.confirm property |
| frontend/package.json | Moved @types/hotwired__turbo to devDependencies and updated version |
| frontend/package-lock.json | Updated @types/hotwired__turbo version and marked as dev dependency |
| modules/meeting/frontend/module/main.ts | Updated to use official Turbo types from @types/hotwired__turbo |
| frontend/src/app/core/main-menu/submenu.service.ts | Updated to use FrameElement type from @types/hotwired__turbo |
Files not reviewed (1)
- frontend/package-lock.json: Language not supported
| .then((service) => service.confirm({ | ||
| text: { title: I18n.t('js.modals.form_submit.title'), text: message }, | ||
| dangerHighlighting | ||
| })) |
Copilot
AI
Jan 16, 2026
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 I18n global is used without ensuring it's available. While I18n is declared as a global in shims.d.ts, the confirm function could potentially be called before I18n is initialized, leading to a runtime error. Consider accessing I18n through window.I18n to be more explicit about the global access, or add a safety check.
| .then((service) => service.confirm({ | |
| text: { title: I18n.t('js.modals.form_submit.title'), text: message }, | |
| dangerHighlighting | |
| })) | |
| .then((service) => { | |
| const i18n = (window as any).I18n; | |
| const title = i18n ? i18n.t('js.modals.form_submit.title') : 'Confirm'; | |
| return service.confirm({ | |
| text: { title, text: message }, | |
| dangerHighlighting | |
| }); | |
| }) |
| const { detail: { response, visit } } = event; | ||
| event.preventDefault(); | ||
| visit(response.url); | ||
| void visit(response.url, {}); |
Copilot
AI
Jan 16, 2026
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 visit function is being called with an empty options object, but the type signature and expected arguments for this function should be verified. The previous code called visit(response.url) without a second argument, and the addition of {} as a second argument may not be correct or necessary depending on the function signature.
| void visit(response.url, {}); | |
| void visit(response.url); |
| export function confirm( | ||
| message:string, | ||
| formElement:HTMLFormElement, | ||
| submitter?:HTMLButtonElement|HTMLInputElement | ||
| ) { |
Copilot
AI
Jan 16, 2026
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 new custom confirmation dialog functionality lacks test coverage. Given that this is a critical user-facing feature that affects form submissions throughout the application, comprehensive tests should be added covering: successful confirmation, cancellation, different HTTP methods (especially 'delete' for danger highlighting), and integration with ConfirmDialogService.
Use Angular `ConfirmDialogService` for the time-being.
26b30e0 to
ee2f0c9
Compare
ℹ️ One of two possible implementations. See also #21639.
Ticket
https://community.openproject.org/wp/69641
What are you trying to accomplish?
Screenshots
What approach did you choose and why?
Merge checklist