Skip to content

Conversation

chaimann
Copy link
Contributor

@chaimann chaimann commented Jun 24, 2025

Summary

Using turbo-confirm add-on create a UI confirmation modal component to use Turbo-native "data-turbo-confirm" functionality along with custom modal dialog, rather than browser native confirm dialog.

Screenshots

Screenshot 2025-06-24 at 19 46 40 Screenshot 2025-06-24 at 19 48 07 Screenshot 2025-06-25 at 18 41 33

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

Copy link

codecov bot commented Jun 25, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.98%. Comparing base (ee815a5) to head (4060de4).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6295   +/-   ##
=======================================
  Coverage   88.97%   88.98%           
=======================================
  Files         861      862    +1     
  Lines       18416    18422    +6     
=======================================
+ Hits        16386    16392    +6     
  Misses       2030     2030           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@chaimann chaimann force-pushed the admin-confirm-modal branch from e0ab3ca to d2d5a3c Compare June 25, 2025 13:18
@chaimann chaimann force-pushed the admin-confirm-modal branch from d2d5a3c to d54d00a Compare June 25, 2025 13:44
chaimann added 3 commits June 25, 2025 17:13
* adds possibility to conditionally open modal on connect - use stimulus
value instead of Dialog's "open" attribute (applying attribute directly
on dialog element is discouraged by HTML specification
https://developer.mozilla.org/en-US/docs/Web/API/HTMLDialogElement/open#value);

* adds identifier classes to title ".modal-title" and body ".modal-body"
so that turbo-confirm can target them correctly;

* adds "empty:hidden" so that when no content is passed the empty div
does not take space in the modal;
@chaimann chaimann force-pushed the admin-confirm-modal branch 2 times, most recently from 4060de4 to c20f8dc Compare June 25, 2025 16:29
chaimann added 6 commits June 25, 2025 18:46
Updates several components to use "data-turbo-confirm" and new
confirmation dialog instead of native browser confirm.

Removes redundant confirm_controller.js.
Default animation duration was 300ms. It controls how long turbo-control
will wait before restoring default confirm modal content (empty
template). Due to that there could be a condition if you click
fast enough when requesting a different confirm dialog, e.g. in
products table clicking "Delete", closing modal and then clicking
"Deactivate", then modal contents would be overwritten with previous
modal contents.
Since we don't use any kind of animation for modals we can put 0
duration to mitigate that bug.
@chaimann chaimann force-pushed the admin-confirm-modal branch from c20f8dc to 28d04f0 Compare June 25, 2025 16:46
@chaimann chaimann marked this pull request as ready for review June 25, 2025 17:16
@chaimann chaimann requested a review from a team as a code owner June 25, 2025 17:16
@jarednorman
Copy link
Member

This has a conflict that needs addressing.

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

I would love to not use an external gem for a simple task as adding a function that opens a Dialog on data-turbo-confirm https://turbo.hotwired.dev/reference/drive#turbo.config.forms.confirm

We already have a Dialog handler that we use to open dialogs. We should use that instead of an external library that very likely will stop receiving updates in a few months.

@tvdeyen
Copy link
Member

tvdeyen commented Aug 5, 2025

For the time being we can continue to use the native confirm that Turbo is using. No user will see a difference anyway. Maybe there area some relevant changes in this 10 commits PR that we can extract..

@tvdeyen tvdeyen self-assigned this Aug 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

5 participants