Admin confirm modal without external dependency - #6528
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6528 +/- ##
=======================================
Coverage 92.16% 92.16%
=======================================
Files 1036 1037 +1
Lines 21188 21195 +7
=======================================
+ Hits 19527 19534 +7
Misses 1661 1661 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
4c08167 to
d7a84d3
Compare
|
@tvdeyen This should be ready for review now! |
| import { openConfirmModal } from "solidus_admin/confirm_modal" | ||
| import "solidus_admin/web_components/solidus_select" | ||
|
|
||
| Turbo.config.forms.confirm = openConfirmModal |
There was a problem hiding this comment.
I believe Turbo.config.forms was added in turbo-rails 2.0.8. If we add this, we need to update our version constraint to s.add_dependency "turbo-rails", "~> 2.0", ">= 2.0.8".
There was a problem hiding this comment.
I opted to add a fallback here for the previous way to configure this, but let me know if you prefer we bump this version instead.
| @@ -0,0 +1,25 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| # Component wrapper for confirmation dialog to use with rolemodel/turbo-confirm. | |||
| ) do |modal| %> | ||
| <% modal.with_actions do %> | ||
| <form method="dialog"> | ||
| <%= render component("ui/button").new(scheme: :secondary, text: t(".cancel"), class: "confirm-cancel") %> |
There was a problem hiding this comment.
Nothing references the CSS class.
There was a problem hiding this comment.
I removed the custom class here, it doesn't seem like it was needed.
| @@ -0,0 +1,13 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| # @component "ui/modal" | |||
There was a problem hiding this comment.
If I understand correctly, this should be layout/confirm, not ui/modal.
There was a problem hiding this comment.
Good catch, must have been a copy/paste error. I updated this.
* 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;
Updates several components to use "data-turbo-confirm" and new confirmation dialog instead of native browser confirm. Removes redundant confirm_controller.js.
Use plain DOM calls to update the confirm dialog markup and show the confirmation modal. This change also pins the new module in the importmap so the bare specifier resolves. Co-authored-by: Senem Soy <senem@super.gd> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Move DOM manipulation out of confirm_modal.js and into a proper component.js Stimulus controller for layout/confirm, attached via data-controller on a wrapper around the modal. confirm_modal.js is now a thin adapter that looks up the controller instance and calls its `open` method, matching how the rest of the admin's per-component JS is structured (e.g. ui/modal, ui/table). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-authored-by: Senem Soy <senem@super.gd>
d7a84d3 to
daa1d4f
Compare
forkata
left a comment
There was a problem hiding this comment.
@jarednorman I addressed your comments here, feel free to take another look!
| @@ -0,0 +1,13 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| # @component "ui/modal" | |||
There was a problem hiding this comment.
Good catch, must have been a copy/paste error. I updated this.
| import { openConfirmModal } from "solidus_admin/confirm_modal" | ||
| import "solidus_admin/web_components/solidus_select" | ||
|
|
||
| Turbo.config.forms.confirm = openConfirmModal |
There was a problem hiding this comment.
I opted to add a fallback here for the previous way to configure this, but let me know if you prefer we bump this version instead.
| @@ -0,0 +1,25 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| # Component wrapper for confirmation dialog to use with rolemodel/turbo-confirm. | |||
| ) do |modal| %> | ||
| <% modal.with_actions do %> | ||
| <form method="dialog"> | ||
| <%= render component("ui/button").new(scheme: :secondary, text: t(".cancel"), class: "confirm-cancel") %> |
There was a problem hiding this comment.
I removed the custom class here, it doesn't seem like it was needed.
`Turbo.config` was introduced in version 8.0.6[^1]. This change ensures that apps using older versions of Turbo can still get the custom confirmation modal behaviour and not error out. [^1]: https://github.com/hotwired/turbo/releases/tag/8.0.6
daa1d4f to
827680e
Compare
Summary
This change addresses the concerns raised in the original PR #6295 re: adding an additional dependency.
In this change we dropped the dependency on https://github.com/RoleModel/turbo-confirm and instead wire the new confirm component directly. There are two additional changes in this PR from the original, the first is just a plain JS function to achieve this, and the last commit tries to refactor that into a Stimulus JS controller. The final approach still uses query selectors to target the dialog title and body but we could potentially refactor the the modal UI component to allow us to pass Stimulus target references down. We did not go as far as doing that in this PR.
We also noticed that our modal component has a
data-controllerreference on it, but we do not have a modal controller defined for that so we've removed it in the last commit to reduce confusion and allow the new parent controller to not conflict with that if we do decide to inject targets in the future.Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The following are not always needed: