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

Raise exception when no id is passed to the turbo_frame_tag helper #551

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

Conversation

thiagopradi
Copy link

Hi Folks,

This PR was created after I went through a debug session to see why one of my Turbo frames was not working correctly. I've misspelled one of the instance variables, and Turbo created a frame with an empty id.

After thinking for a little bit, I couldn't think of a use case where it's expected/intended to create a turbo frame with an empty tag, so I created a pull request that raises ArgumentError if a valid ID is not passed. (please correct me If there's any use case where we expect to create a turbo frame with empty IDs).

Thiago

@thiagopradi
Copy link
Author

@seanpdoyle great suggestion - PR updated.

@thiagopradi
Copy link
Author

@seanpdoyle - let me know if you need any other updates to the PR.

@seanpdoyle
Copy link
Contributor

@jorgemanrubia could you approve this PR so for CI?

Copy link
Contributor

@brunoprietog brunoprietog left a comment

Choose a reason for hiding this comment

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

Hi @thiagopradi, thanks for your contribution! I left a minor suggestion and i'll merge

@@ -21,6 +21,26 @@ class Turbo::FramesHelperTest < ActionView::TestCase
assert_dom_equal %(<turbo-frame id="message_1"></turbo-frame>), turbo_frame_tag(record)
end

test "frame with invalid argument should raise ArgumentError" do
Copy link
Contributor

Choose a reason for hiding this comment

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

The record variables could be ignored here and pass the argument directly. I don't see much value in declaring them

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.

3 participants