Convert the remaining Rails UJS remotes to Turbo#4063
Open
tvdeyen wants to merge 3 commits into
Open
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4063 +/- ##
=======================================
Coverage 98.09% 98.09%
=======================================
Files 347 347
Lines 9016 9016
=======================================
Hits 8844 8844
Misses 172 172 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The prev/next links in the image overlay were Rails UJS remote links whose response was swapped into the dialog by Dialog#watch_remote_forms. Wrapping the picture in a Turbo frame lets Turbo do the same replacement, which is one less call site keeping @rails/ujs alive. Turbo replaces only the frame contents, so the overlay can no longer bind its click handlers to the elements it finds at load time. Delegating them to the dialog body and looking the navigation links up when a key is pressed keeps them working across navigations. Re-running init on turbo:frame-load would have been the smaller change, but that event only fires a repaint after the new content is live, leaving a window in which a click hits an unbound element.
Both copy to clipboard links relied on Rails UJS to turn them into POST requests: the layoutpage one via data-remote, the page list one via data-method, since the data-turbo it carried is not an attribute Turbo honours. The clipboard controller already answers with a turbo stream, so a form is all they need, and it comes with Turbo's protection against a double submit. The sitemap already copies pages this way. In the page list the surrounding cell lays its tools out inline, so the form gets the same treatment as the delete button next to it.
The form is submitted from the select's onchange handler, and a programmatic form.submit() fires no submit event, so neither Rails UJS nor Turbo ever saw it: the browser has always sent the request itself. The remote option was therefore never in effect, and nothing consumed a response for it, which the added spec pins down.
tvdeyen
force-pushed
the
ujs-remotes-to-turbo
branch
from
a74d30a to
3559923
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is this pull request for?
A further step towards removing
@rails/ujsfrom the admin. Four call sites still leaned on it, and none of them needed to: the clipboard controller already answers with a turbo stream, and the picture overlay only ever used the remote response to swap the dialog content, which is what a Turbo frame does.The picture overlay now renders its picture inside a Turbo frame that the prev/next links target. Because Turbo replaces the frame contents rather than the whole dialog body, the overlay can no longer bind its click handlers once at load time, so they are delegated to the dialog body instead. Re-initializing on
turbo:frame-loadlooked like the smaller change but is subtly broken: Turbo dispatches that event a repaint after the new content is already live, so a quick click lands on an element that is not bound yet — a system spec caught this reliably.The two copy to clipboard links become
button_toforms. Both were only doing a POST by grace of UJS (data-remoteon the layoutpage,data-methodon the page list — thedata: {turbo: true}there did nothing, Turbo only honoursdata-turbo-method), and a form additionally gets Turbo's protection against a double submit. The sitemap already copies pages this way.The per page select turned out to need nothing but the removal of a dead option: it is submitted from the select's
onchangehandler, and a programmaticform.submit()fires no submit event, so neither UJS nor Turbo ever saw that form. The browser has always sent the request itself.Notable changes (remove if none)
Dialog#watch_remote_formsstays for now. The picture and file assign links inside the archive overlays are still UJS remote links answering withassign.js.erb, so[data-remote="true"]can still show up in a dialog body. It can go once those are converted, together with@rails/ujsitself.Screenshots
No visual changes. The rendered boxes were measured before and after: in the page list the copy icon sits at the same coordinates as the link it replaces (x=973, 16x16, between the settings and delete icons), the layoutpage row is pixel identical, and the overlay's picture, navigation, details panel and handle keep their exact geometry.
Checklist