Skip to content

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

Co-authored-by: Sean Doyle <seanpdoyle@users.noreply.github.com>
@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

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

3 participants