Skip to content
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 turbo-confirm for links without a turbo-method #1266

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

tpaulshippy
Copy link

@tpaulshippy tpaulshippy commented May 26, 2024

Resolves #1264

It appears that the turbo-confirm feature implementation relies on the link being captured by the FormLinkClickObserver rather than the LinkClickObserver. Without a data-turbo-method (or data-turbo-stream) attribute, the FormLinkClickObserver will ignore the link and the confirm feature will not fire.

This small bug fix adds the data-turbo-confirm attribute to the other two attributes that indicate to the FormLinkClickObserver that it should handle the link.

@tpaulshippy
Copy link
Author

tpaulshippy commented May 26, 2024

Seeing one flaky test failure.

  [firefox] › functional/navigation_tests.js:432:1 › does not fire turbo:load twice after following a redirect 

But it looks like this is happening in the main branch as well.

@tpaulshippy
Copy link
Author

tpaulshippy commented May 26, 2024

#379 (comment)
Looks like the current behavior was intended and by design. If this is still the case, perhaps we just need an update to https://github.com/hotwired/turbo-site/blob/main/_source/handbook/02_drive.md with this detail.

It does seem odd to me that data-turbo-method="GET" is necessary to make data-turbo-confirm work.

@pinzonjulian
Copy link

pinzonjulian commented May 26, 2024

The original intention of the confirm dialog as I understand it is to alert the user about operations that will modify the state of the system. In the case of the web that's requests with a PUT/PATCH/DELETE verb.

A GET request, which is how links work by default, shouldn't modify your system so a confirmation seems odd.

When you explicitly tell a link to use another method (a technique that's more and more uncommon these days) then you are allowed to ask the user for confirmation.

What's your use case for asking a user for confirmation on a GET request?

@tpaulshippy
Copy link
Author

I don't personally have a use case. I'm sure @JaceBayless does.

But the one documented at https://turbo.hotwired.dev/handbook/drive#requiring-confirmation-for-a-visit seems reasonable enough to me: "Do you want to leave this page?"

@tpaulshippy
Copy link
Author

tpaulshippy commented May 26, 2024

Actually a very common use case just happened for me. I started to edit my comment, and then decided to make a new comment. When I clicked "Cancel," Github asked me to confirm that I wanted to discard my changes.

Any Cancel click on a dirty new/edit form will not modify the system at all and could be constructed as a GET, but a confirmation is very appropriate.

@pinzonjulian
Copy link

You're right! That's a great use case.

@tpaulshippy
Copy link
Author

tpaulshippy commented May 26, 2024

The more I think about it, the more I think this implementation doesn't really fit that particular use case. For a couple of reasons: 1) You generally want to do a dirty check in these cases which would usually require additional Javascript before deciding to confirm. 2) You would want the confirm to fire on close of the window/tab as well as any navigation away so a beforeunload event would better cover that. Would be nice if Turbo could handle that but I'm not sure how.

There still could be other use cases for a confirm on navigate though. Maybe you have a set of filter controls and the user has built up a set of filters. Then you have a Clear Filters button that just navigates back to the list action without query strings. You might only show that button if query strings exist (the "dirty check"). In that case you probably wouldn't want to confirm on close of the window because there isn't really a potential for data loss.

I'm sure there are others.

- These were for illustrating the workaround, but probably aren't needed in the suite longer term
@JaceBayless
Copy link

In what I was doing, I didn't have a great use case. I was testing some things out and noticed that the documentation didn't match the actual behavior.

FWIW I think one of the main uses for this would be if you're trying to prevent people from navigating to external sites, generally they do this with a warning message.

@marcoroth
Copy link
Member

marcoroth commented May 29, 2024

FWIW, this is a duplicate of #874

@tpaulshippy
Copy link
Author

True. The implementation approach is quite different though. How would you compare them?

@marcoroth
Copy link
Member

I'm wondering if your approach also works for form submissions

@tpaulshippy
Copy link
Author

Were form submissions broken? I thought it was just links.

@tpaulshippy
Copy link
Author

I think my approach just turns the links into form submissions if they have a confirmation.

searls added a commit to searls/turbo-site that referenced this pull request Jul 5, 2024
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.
searls added a commit to searls/turbo-site that referenced this pull request Jul 5, 2024
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.
@tpaulshippy
Copy link
Author

@marcoroth
Still curious about your opinion of this approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

turbo-confirm doesn't work on links without a turbo-method
4 participants