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

feature: allow actions to render turbo streams #2796

Merged
merged 8 commits into from
Jul 12, 2024

Conversation

Paul-Bob
Copy link
Contributor

@Paul-Bob Paul-Bob commented May 22, 2024

Description

Fixes #2790

This PR enables enhancing action response by adding custom turbo streams.

def handle(**args)
  # ...

  # Single
  append_to_response -> {
    turbo_stream.set_title("Dummy action custom stream ;)")
  }

  # Array
  append_to_response -> {
    [
      turbo_stream.set_title("Cool title"),
      turbo_stream.set_title("Cool title 2")
    ]
  }
end

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

Screenshots & recording

Manual review steps

  1. Step 1
  2. Step 2

Manual reviewer: please leave a comment with output from the test if that's the case.

@Paul-Bob Paul-Bob self-assigned this May 22, 2024
@Paul-Bob Paul-Bob marked this pull request as draft May 22, 2024 16:50
Copy link

codeclimate bot commented May 22, 2024

Code Climate has analyzed commit 5b269f4 and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1

View more on Code Climate.

lib/avo/base_action.rb Outdated Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Jun 7, 2024

This PR has been marked as stale because there was no activity for the past 15 days.

Copy link
Contributor

This PR has been marked as stale because there was no activity for the past 15 days.

@github-actions github-actions bot added the Stale label Jun 27, 2024
@Paul-Bob Paul-Bob removed the Stale label Jul 5, 2024
@Paul-Bob Paul-Bob marked this pull request as ready for review July 8, 2024 09:34
Copy link
Collaborator

@adrianthedev adrianthedev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rename it to respond_with and left a few tweaks

Comment on lines 14 to 17
enhance_response turbo_stream: -> {
turbo_stream.set_title("Dummy action custom stream ;)")
}
end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rename it to respond_with and pass in an array or a single turbo_stream action.

The reason why I'm thinking to not do this turbo_stream: -> si because we only respond with turbo streams... It's a bit redundant to declare it every time, right?
Do you see any caveats?

Suggested change
enhance_response turbo_stream: -> {
turbo_stream.set_title("Dummy action custom stream ;)")
}
end
enhance_response turbo_stream: -> {
turbo_stream.set_title("Dummy action custom stream ;)")
}
end

Copy link
Contributor Author

@Paul-Bob Paul-Bob Jul 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rename it to respond_with ...

respond_with kinda sounds like is overriding the default responses.

enhance_response - "Add this to response"
respond_with - "Respond with this`

...and pass in an array or a single turbo_stream action.
The reason why I'm thinking to not do this turbo_stream: -> si because we only respond with turbo streams... It's a bit redundant to declare it every time, right?
Do you see any caveats?

You mean something like (I'll use enhance_response until renaming):

enhance_response turbo_stream: turbo_stream.set_title("Dummy action custom stream ;)")

# OR

enhance_response turbo_stream.set_title("Dummy action custom stream ;)")

And for array:

enhance_response turbo_stream: [
  turbo_stream.set_title("Dummy action custom stream ;)"),
  turbo_stream...
]

# OR

enhance_response [
  turbo_stream.set_title("Dummy action custom stream ;)"),
  turbo_stream...
]

Do you see any caveats?

1 - not sure if turbo_stream helper method is present.
2 - no more controller response context

end

responses = if @action.enhanced_turbo_streams.present?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't we include this if statement in the case statement above?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The scope of this feature is to add one or several turbo-stream responses to the pre-existing response

def handle(**args)
    succeed "Modal closed!!"
    close_modal
    enhance_response turbo_stream: -> {
      turbo_stream.set_title("Dummy action custom stream ;)")
    }
  end

For example on the code above we want the success message to be rendered, the modal to be closed then enhance that response with the turbo_stream.set_title("Dummy action custom stream ;)")

If we add the if statement on the case above the action would respond only with turbo_stream.set_title("Dummy action custom stream ;)") ignoring succeed "Modal closed!!" and close_modal responses.

@Paul-Bob
Copy link
Contributor Author

Naming suggestions:
1 - respond_with
2 - add_to_response
3 - append_to_response
4 - attach_to_response
5 - merge_with_response
6 - include_in_response
7 - insert_into_response
8 - concatenate_response

@adrianthedev
Copy link
Collaborator

I like append_to_response @Paul-Bob. Let's push it!

spec/system/avo/actions_spec.rb Outdated Show resolved Hide resolved
spec/system/avo/actions_spec.rb Outdated Show resolved Hide resolved
@Paul-Bob
Copy link
Contributor Author

@adrianthedev changes & docs applied

@@ -196,6 +197,7 @@
click_on "Run"
expect(page).not_to have_selector(modal)
expect(page).to have_text "Modal closed!!"
expect(page).to have_title("Cool title")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[rubocop] reported by reviewdog 🐶
[Corrected] Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@@ -196,6 +197,7 @@
click_on "Run"
expect(page).not_to have_selector(modal)
expect(page).to have_text "Modal closed!!"
expect(page).to have_title("Cool title")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[rubocop] reported by reviewdog 🐶
[Corrected] Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

Comment on lines +12 to +14
append_to_response -> {
turbo_stream.set_title("Cool title")
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't we settled on using an array notation?
I mean, why the extra block?

Suggested change
append_to_response -> {
turbo_stream.set_title("Cool title")
}
append_to_response([
turbo_stream.set_title("Cool title")
])

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could do

append_to_response do
  turbo_stream...
end

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forget about it. A block is enough. I hope users don't mistake it for the Execution Context.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LEt's also discuss this possibility

    # turbo_stream_response will be executed instead the default `render turbo_stream: responses` from actions controller
    turbo_stream_response -> {
      render turbo_stream: [
        *default_responses, # can include or not the default responses
        turbo_stream.set_title("Cool title"),
        turbo_stream....
      ]
    }

@Paul-Bob Paul-Bob merged commit b396c12 into main Jul 12, 2024
19 of 21 checks passed
@Paul-Bob Paul-Bob deleted the feature/allow_actions_to_render_turbo_streams branch July 12, 2024 13:30
Copy link
Contributor

This PR has been merged into main. The functionality will be available in the next release.

Please check the release guide for more information.

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

Successfully merging this pull request may close these issues.

Add the ability to respond with turbo_streams from actions
2 participants