Skip to content

Conversation

@reeganviljoen
Copy link
Collaborator

@reeganviljoen reeganviljoen commented Jan 8, 2024

closes #1955

What are you trying to accomplish?

When an integer or any non-string is used inside call the html escaping throw an error(described in the linked issue)

What approach did you choose and why?

I replaced text.nil? || text.empty? for with type safe .blank? so any type can be escaped as is allowed in templates

@reeganviljoen reeganviljoen changed the title Rv fix html escapping Fix html escaping on non strings Jan 8, 2024
Copy link
Contributor

@camertron camertron left a comment

Choose a reason for hiding this comment

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

Thanks for taking a look at this @reeganviljoen!

end
end

def test_inline_slot_html_escape_with_integer
Copy link
Contributor

Choose a reason for hiding this comment

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

These tests don't appear to assert anything?

def test_inline_slot_html_escape_with_integer
render_inline InlineSlotHtmlEscapeComponent.new(heading: "lorem ipsum", counter: 12, url: "/some_url") do |component|
component.with_empty_state do
"empty state"
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense for this slot to return a non-string value? Otherwise we're testing the existing behavior and not the behavior mentioned in the issue.

@camertron
Copy link
Contributor

Hey @reeganviljoen, in the interest of getting a release out today I forked your branch and created another PR with a few small fixes. Thanks for tackling this!!

@camertron camertron closed this Jan 8, 2024
@reeganviljoen
Copy link
Collaborator Author

@camertron sorry I didd't see this as I was in the gym 😄

@camertron
Copy link
Contributor

@reeganviljoen no worries! Sorry for closing, but your name is still in the changelog for this fix!

@reeganviljoen
Copy link
Collaborator Author

@camertron no problem I fully understand the urgency, just letting you know I wasn't blowing off your response

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

#maybe_escape_html chokes when input is an Integer

3 participants