-
Notifications
You must be signed in to change notification settings - Fork 435
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
Fix data-turbo-confirm
on <a>
without data-turbo-method
#874
base: main
Are you sure you want to change the base?
Fix data-turbo-confirm
on <a>
without data-turbo-method
#874
Conversation
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.
I agree with the tests and the underlying behavior here.
Having said that, I worry that the FormLinkClickObserver
is an inappropriate candidate for supporting that behavior.
Would an <a>
without [data-turbo-method]
be treated as a typical Visit navigation?
In the absence of that attribute, what is directing the code to flow through this observer? Could confirmation be accommodated elsewhere, closer to a typical <a>
-driven Visit?
634dd95
to
2bf844d
Compare
Thanks @seanpdoyle, this is good feedback! You are right, this logic doesn't belong to the I pushed up a version where we check for the presence of the Since |
2bf844d
to
b9bc188
Compare
Seems weird that the documentation still says this is possible when the example cited doesn't actually work:
Just spent 15 minutes verifying it wasn't a problem in my turbo version or import maps setup |
Let me get this rebased so we get a chance to get this this merged in |
b9bc188
to
c8f05e9
Compare
c8f05e9
to
f3682bd
Compare
Until hotwired/turbo#874 or hotwired/turbo#1266 resolve hotwired/turbo#1264 and hotwired/turbo#943, the doc site is wrong and I keep wasting time double-checking it, only for its wrongness to result in me wasting more time debugging Turbo's source code.
Until hotwired/turbo#874 or hotwired/turbo#1266 resolve hotwired/turbo#1264 and hotwired/turbo#943, the doc site is wrong and I keep wasting time double-checking it, only for its wrongness to result in me wasting more time debugging Turbo's source code.
f3682bd
to
4c3c658
Compare
I just rebased this PR again, but I feel like this is not quite right anymore after the merged PR #1217 from @seanpdoyle. The new I'm wondering if we should either rename the config option or also use the |
4c3c658
to
bbd65cc
Compare
The documentation in the "Requiring Confirmation for a Visit" section mentions the use of the
[data-turbo-confirm]
attribute on an<a>
element to prompt a confirmation before executing a visit. An example is given as follows:Prior to version
v7.2.5
, this functionality did not work unless the[data-turbo-method]
attribute was also present on the link.This pull request modifies the behavior so that
[data-turbo-confirm]
can now be used on links without the[data-turbo-method]
attribute.Resolves #943
Resolves #1264
Resolves #1296
Closes #1266