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

Disabling Drive keeps Frames default behaviour #648

Merged
merged 1 commit into from
Jul 29, 2022

Conversation

rik
Copy link
Contributor

@rik rik commented Jul 25, 2022

#196 introduced the ability to disable Drive by default. An unintentional side-effect was that it also disabled Frame by default. Every Frame needed the data-turbo attribute to be enabled: <turbo-frame data-turbo="true">. This makes it so that Turbo.session.drive = false only affects Drive.

On top of the logic change, we rename the Session methods to remove the Drive vocabulary.


@gregschmit Ping as the author of #196. Am I right to assume that this was an un unintentional side-effect?

hotwired#196 introduced the ability to disable Drive by default. An unintentional side-effect was that it also disabled Frame by default. Every Frame needed the data-turbo attribute to be enabled: `<turbo-frame data-turbo="true">`. This makes it so that `Turbo.session.drive = false` only affects Drive.

On top of the logic change, we rename the Session methods to remove the Drive vocabulary.
@rik rik force-pushed the disabling-drive-keeps-frames-default branch from f793915 to af53df2 Compare July 25, 2022 19:03
@gregschmit
Copy link
Contributor

Yes, that was definitely unintentional. I thought that since frames are opt-in already (in the sense that you have to explicitly use the turbo-frame element), that they wouldn't be impacted by the data-turbo element property to determine whether they should function as frames (but I see now that they are looking at elementIsNavigatable).

This change makes a lot of sense to me, because it essentially says that if you've inserted a turbo-frame element, then turbo should be enabled in that context, even if you disabled it by default.

@dhh dhh added this to the 7.2.0 milestone Jul 28, 2022
@dhh dhh merged commit 567edf0 into hotwired:main Jul 29, 2022
marcoroth added a commit to marcoroth/turbo that referenced this pull request Jul 29, 2022
…le()`

The Pull Request hotwired#648 refactored the function name of
`session.elementDriveEnabled()` to `session.elementIsNavigatable()`.

This Pull Request fixes an old function reference which kept the library
from building successfully.
dhh pushed a commit that referenced this pull request Jul 29, 2022
…le()` (#656)

The Pull Request #648 refactored the function name of
`session.elementDriveEnabled()` to `session.elementIsNavigatable()`.

This Pull Request fixes an old function reference which kept the library
from building successfully.
@rik rik deleted the disabling-drive-keeps-frames-default branch July 30, 2022 13:35
@guillaumewrobel
Copy link

Breaking change in our codebase, but we'll add target="_top" or data-turbo="false" to all links inside <turbo-frame-tag>

@hidr0
Copy link

hidr0 commented Sep 26, 2022

This broke something for me too.

@gregschmit could you please humour me and explain how:

This change makes a lot of sense to me, because it essentially says that if you've inserted a turbo-frame element, then turbo should be enabled in that context, even if you disabled it by default.

should be enabled in that context, even if you disabled it by default. isn't default needed to be default?

Thinking out loud here, I kind of get your point, but in this way there is not easy one switch to disabled Drive application wide. Do we need that one switch, hmm.. Not sure.

Maybe we need a flag to disable Drive + Frames?

Just raising a flag here.

seanpdoyle referenced this pull request Oct 18, 2022
Closes #670

Re-use the existing `2xx` and `3xx` behavior for a response to handle
error responses. When the frame is missing from the page (likely for
error pages like `404`), dispatch a `turbo:frame-missing` event.

Alongside that behavior, yield the [Response][] instance and a
`Turbo.visit`-like callback that can transform the `Response` instance
into a `Visit`. It also accepts all the arguments that the `Turbo.visit`
call normally does.

When the event is not canceled, treat the `Response` as a Visit. When
the event **is** canceled, let the listener handle it.

[Response]: https://developer.mozilla.org/en-US/docs/Web/API/Response
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.

5 participants