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

Turbo::StreamsChannel can set targets attribute on stream tag #210

Merged
merged 2 commits into from
Aug 26, 2021

Conversation

ceritium
Copy link
Contributor

Background:

This attempts to expose the feature on Turbo::StreamsChannel methods.

@ceritium ceritium marked this pull request as ready for review July 29, 2021 18:06
@dhh
Copy link
Member

dhh commented Aug 26, 2021

👍. Can you add some tests for this as well?

Background:
- hotwired/turbo#113 allowed for multiple targets in turbo frame streams.
- hotwired#194 implemented it for `Turbo::Streams::TagBuilder`

This attempts to expose the feature on `Turbo::StreamsChannel` methods.
@ceritium
Copy link
Contributor Author

Hi @dhh, sure I can do it.

I have three different approaches in mind:

  • Just duplicate some of the tests cases with the targets argument.
  • Add another assertion in some tests cases with the targets argument.
  • Wrap some tests cases with a method to iterate over target and targets arguments.

Something like:

    with_target_options do |opts|
      assert_broadcast_on "stream", turbo_stream_action_tag("replace", template: "<p>hello!</p>", **opts) do
        Turbo::StreamsChannel.broadcast_replace_to "stream", partial: "messages/message", locals: { message: "hello!" }, **opts
      end
    end

I think covering the uses cases that match with broadcasting (remove/replace/update/append/prepend) (now/later) is enogh.

What do you think?

@ceritium ceritium mentioned this pull request Aug 26, 2021
@dhh
Copy link
Member

dhh commented Aug 26, 2021

Think just adding another assert with the targets argument is fine. Doesn't need to be too clever!

@ceritium ceritium force-pushed the broadcasts-targets branch 3 times, most recently from 550cba5 to 795a3b3 Compare August 26, 2021 15:27
@dhh dhh merged commit ef48bde into hotwired:main Aug 26, 2021
@ceritium ceritium deleted the broadcasts-targets branch August 26, 2021 15:36
ghiculescu added a commit to ghiculescu/turbo-rails that referenced this pull request Dec 9, 2022
hotwired#210 added the ability to set `targets` on a stream tag. But that doesn't work nicely with the `Broadcastable` helper methods. Currently you have to do this to target some `targets`:

```ruby
after_update_commit -> { broadcast_update_to self, target: nil, targets: ".class_name" }
```

This PR improves things so that you don't need to provide `target: nil` anymore.
ghiculescu added a commit to ghiculescu/turbo-rails that referenced this pull request Dec 9, 2022
hotwired#210 added the ability to set `targets` on a stream tag. But that doesn't work nicely with the `Broadcastable` helper methods. Currently you have to do this to target some `targets`:

```ruby
after_update_commit -> { broadcast_update_to self, target: nil, targets: ".class_name" }
```

This PR improves things so that you don't need to provide `target: nil` anymore.
ghiculescu added a commit to ghiculescu/turbo-rails that referenced this pull request Dec 9, 2022
hotwired#210 added the ability to set `targets` on a stream tag. But that doesn't work nicely with the `Broadcastable` helper methods. Currently you have to do this to target some `targets`:

```ruby
after_update_commit -> { broadcast_update_to self, target: nil, targets: ".class_name" }
```

This PR improves things so that you don't need to provide `target: nil` anymore.
ghiculescu added a commit to ghiculescu/turbo-rails that referenced this pull request Dec 9, 2022
hotwired#210 added the ability to set `targets` on a stream tag. But that doesn't work nicely with the `Broadcastable` helper methods. Currently you have to do this to target some `targets`:

```ruby
after_update_commit -> { broadcast_update_to self, target: nil, targets: ".class_name" }
```

This PR improves things so that you don't need to provide `target: nil` anymore.
JuannFerrari added a commit to JuannFerrari/turbo-rails that referenced this pull request Nov 9, 2023
hotwired#210 added the ability to set `targets` on a stream tag. But that doesn't work nicely with the `Broadcastable` helper methods. Currently you have to do this to target some `targets`:

```ruby
after_update_commit -> { broadcast_update_to self, target: nil, targets: ".class_name" }
```

This PR improves things so that you don't need to provide `target: nil` anymore.

--

This PR is a continuation of the work made by @ghiculescu at PR turbo-rails#408
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

2 participants