Skip to content

Slots return stripped HTML #414

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 21, 2020
Merged

Slots return stripped HTML #414

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# master

* Slots return stripped HTML, removing leading and trailing whitespace.

*Jason Long, Joel Hawksley*

# 2.16.0

* Add `--sidecar` option to the erb, haml and slim generators. Places the generated template in the sidecar directory.
Expand Down
2 changes: 1 addition & 1 deletion coverage/coverage.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,4 @@ Incomplete test coverage
/lib/view_component/preview.rb: 94.0% (missed: 47,57,107)
/lib/view_component/render_to_string_monkey_patch.rb: 83.33% (missed: 9)
/lib/view_component/test_helpers.rb: 91.67% (missed: 17,25)
/test/view_component/integration_test.rb: 96.69% (missed: 14,15,16,18,20,365,366,367,369)
/test/view_component/integration_test.rb: 96.73% (missed: 14,15,16,18,20,370,371,372,374)
2 changes: 1 addition & 1 deletion lib/view_component/slotable.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ def slot(slot_name, **args, &block)
slot_instance = args.present? ? slot_class.new(**args) : slot_class.new

# Capture block and assign to slot_instance#content
slot_instance.content = view_context.capture(&block) if block_given?
slot_instance.content = view_context.capture(&block).strip.html_safe if block_given?
Copy link
Contributor

Choose a reason for hiding this comment

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

Will .capture always return html_safe? content?

I'm curious if it'd make sense to check if the captured content is html_safe? and only apply html_safe if it was already marked as HTML safe.

Copy link
Member Author

Choose a reason for hiding this comment

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

@BlakeWilliams capture returns an ActiveSupport::SafeBuffer, so I think we're fine to assume that it is safe.


if slot[:collection]
# Initialize instance variable as an empty array
Expand Down
5 changes: 5 additions & 0 deletions test/view_component/integration_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,11 @@ class IntegrationTest < ActionDispatch::IntegrationTest
assert_select(".item.normal", count: 2)

assert_select(".footer.text-blue h3", text: "This is the footer")

title_node = Nokogiri::HTML.fragment(response.body).css(".title").to_html
expected_title_html = "<div class=\"title\">\n <p>This is my title!</p>\n </div>"
Copy link
Member

Choose a reason for hiding this comment

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

What would we have to do to get this to succeed?

Suggested change
expected_title_html = "<div class=\"title\">\n <p>This is my title!</p>\n </div>"
expected_title_html = "<div class=\"title\"><p>This is my title!</p></div>"

Copy link
Member Author

Choose a reason for hiding this comment

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

@jonrohan we'd have to strip the rendered output for the component itself. It's certainly doable 😄

Copy link
Member

Choose a reason for hiding this comment

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

The browser wouldn't treat these differently, but they are 10 bytes in size difference.

Copy link

@jduff jduff Jul 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I recently made some changes to assert_dom_equal so that it ignores white space between elements. I’ve been using it to test the output of my view components and it has been working well. It’s in master but not a released version of the gem, but might be something to look at for this rails/rails-dom-testing#84


assert_equal(title_node, expected_title_html)
end

if Rails.version.to_f >= 6.1
Expand Down