BREAKING: Allow with_content to override content passed as a block. #2346
Conversation
| view_context.capture(self, &@__vc_render_in_block) | ||
| elsif __vc_content_set_by_with_content_defined? | ||
| if __vc_content_set_by_with_content_defined? | ||
| @__vc_content_set_to_with_content_value = true |
There was a problem hiding this comment.
My brain is having a really hard time parsing what this variable's name means. Is there a better name we can use? Idea: @__vc_content_set_via_static_value
There was a problem hiding this comment.
I'm also having a hard time reading this code, I think because of how similar the variable names are here. Maybe there's shorter names we could apply?
| * BREAKING: Allow `with_content` to override content passed as a block. `DuplicateContentError` and `DuplicateSlotContentError` will no longer be raised. | ||
|
|
||
| For example, components and slots can now have their content set with `with_content` even when used with block syntax: | ||
|
|
||
| ```ruby | ||
| render(ParentSlotComponent.new.with_content("Parent Content")) do |parent| | ||
| parent.with_child do |child| | ||
| child.with_title { "Child Title" } | ||
| child.with_content("Child Content") | ||
| end | ||
| end | ||
| ``` | ||
|
|
||
| *Joel Hawksley* | ||
|
|
There was a problem hiding this comment.
I would love to see what the rendered result of this is in the docs, i.e. show that slots are defined and the content provided with with_content also renders.
| class DuplicateContentError < StandardError | ||
| MESSAGE = | ||
| "It looks like a block was provided after calling `with_content` on COMPONENT, " \ | ||
| "which means that ViewComponent doesn't know which content to use.\n\n" \ | ||
| "To fix this issue, use either `with_content` or a block." | ||
|
|
||
| def initialize(klass_name) | ||
| super(MESSAGE.gsub("COMPONENT", klass_name.to_s)) | ||
| end | ||
| end | ||
|
|
There was a problem hiding this comment.
Do we really want to get rid of this error? If you call with_content more than once, is it ok that the previous value gets overwritten with no warning? Fine either way, but is that behavior documented anywhere?
There was a problem hiding this comment.
I understand the approach here, but I can't help but feel this is a workaround our existing API that the "content as a slot" solution would fix. Ultimately, it seems problematic to continue conflating executing API code in the block with the block returning content.
If we separated the two, we could keep our error messages around duplicative slot content and have a (potentially) simpler implementation.
| view_context.capture(self, &@__vc_render_in_block) | ||
| elsif __vc_content_set_by_with_content_defined? | ||
| if __vc_content_set_by_with_content_defined? | ||
| @__vc_content_set_to_with_content_value = true |
There was a problem hiding this comment.
I'm also having a hard time reading this code, I think because of how similar the variable names are here. Maybe there's shorter names we could apply?
| @@ -5,6 +5,7 @@ module WithContentHelper | |||
| def with_content(value) | |||
| raise NilWithContentError if value.nil? | |||
|
|
|||
| @__vc_content_set_to_with_content_value = false | |||
There was a problem hiding this comment.
Why does this need to be called?
Having sat on this PR for the weekend, I agree. I was kind of hoping this PR might help us avoid the content-as-a-slot work, but I think it's too much of a half-measure. Closing. |
For example, slots can now have their content set with
with_content:In practice, this addresses the case where
with_contentwas disallowed due to the block passed towith_child, despite the block not providing content.Closes #2345