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

Treat [data-turbo-stream] as boolean attribute #630

Merged
merged 1 commit into from Jul 17, 2022

Conversation

seanpdoyle
Copy link
Contributor

Replace equality checks for [data-turbo-stream="true"] with presence
checks for [data-turbo-stream] to more closely match the canonical
behavior for boolean HTML attributes.

Turbo already supports other boolean attributes like
[data-turbo-permanent] and [data-turbo-confirm], so
[data-turbo-stream] should match the precedent.

@seanpdoyle
Copy link
Contributor Author

@kevinmcconnell this is a follow-up to #612.

If this ships, could you update hotwired/turbo-site#103 to reflect the boolean nature of the attribute?

@seanpdoyle seanpdoyle force-pushed the data-turbo-stream-boolean-attribute branch from 308035a to 095992c Compare July 16, 2022 16:47
@dhh
Copy link
Member

dhh commented Jul 17, 2022

Looks good to me. Can you rebase?

Replace equality checks for `[data-turbo-stream="true"]` with presence
checks for `[data-turbo-stream]` to more closely match the canonical
behavior for [boolean HTML attributes][].

Turbo already supports other boolean attributes like
`[data-turbo-permanent]` and `[data-turbo-confirm]`, so
`[data-turbo-stream]` should match the precedent.

[boolean HTML attributes]: https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes#boolean_attributes
@seanpdoyle seanpdoyle force-pushed the data-turbo-stream-boolean-attribute branch from 095992c to f9895d6 Compare July 17, 2022 22:54
@dhh dhh merged commit 98f81c4 into hotwired:main Jul 17, 2022
@seanpdoyle seanpdoyle deleted the data-turbo-stream-boolean-attribute branch July 17, 2022 23:30
@evgenyneu
Copy link

I want "data-turbo-stream" attribute in my form with POST to be "false", so it does not request turbo stream response, but still uses turbo (without page reload). Is it possible?

@marcoroth
Copy link
Member

I want "data-turbo-stream" attribute in my form with POST to be "false"

In that case you can just omit the data-turbo-stream attribute on your form.

@kevinmcconnell
Copy link
Collaborator

kevinmcconnell commented Jul 21, 2023

@evgenyneu I don't think that's possible right now, unfortunately. We don't have a way to opt out of stream responses for POST forms, as that's the default behaviour.

Could you handle this on the server side instead, by not serving a turbo stream response for that case?

@evgenyneu
Copy link

@kevinmcconnell, do we want ability to opt out of turbo streams for POST forms? If yes I can create a PR, so we can use data-turbo-stream="false" for forms. But this will basically revert changes in this PR? I'm confused.

Could you handle this on the server side instead, by not serving a turbo stream response for that case?

Thanks, good idea, I can add a "no-turbo-stream" parameter to my form, and on server side render a standard html.erb view (instead turbo_stream.erb`) if it's present.

@kevinmcconnell
Copy link
Collaborator

do we want ability to opt out of turbo streams for POST forms? If yes I can create a PR, so we can use data-turbo-stream="false" for forms.

It sounds like a good idea to me. I've not personally run into a situation where I needed this, but I could see how it could be useful.

But this will basically revert changes in this PR?

It will, but there's a little more to it, too. The default behaviour is to accept stream responses for POST but not GET. And we have a way to opt-in to use them for all methods. If you also add a way to opt-out for all methods, there are now 3 settings. I think the clearest way that's also backwards compatible would be to turn data-turbo-stream into an enumerated attribute, with 3 values:

  • auto - only accept stream responses for POST (the default, if not specified)
  • true - accept stream responses regardless of method (also the "missing value default", so you can set true just by adding data-turbo-stream without a value)
  • false - don't accept stream responses

What do you think? Or did you have another way in mind?

If you want to make a PR to solve it, I'd be happy to review when it's ready.

@evgenyneu
Copy link

@kevinmcconnell sounds like a good plan, I'll make a PR. What's the purpose of the auto value, if it's the same as the default (missing data-turbo-stream attribute)?

@kevinmcconnell
Copy link
Collaborator

I don't think you'd actually need to set auto in practice, I mostly just wanted to list the 3 states there for clarity :) That said, it might be clearer in the code to actually track these 3 states by name, and allow any of its values to specified explicitly for consistency. But it's up to you if you feel that helps or not in the PR.

@MatheusRich
Copy link

@evgenyneu any updates on that PR? I'm interested in the same feature and wondering if I should work on that.

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.

None yet

6 participants