-
Notifications
You must be signed in to change notification settings - Fork 442
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
Multiple templates per component, allowing arguments #451
base: main
Are you sure you want to change the base?
Multiple templates per component, allowing arguments #451
Conversation
Thanks for the PR! We're starting to think about slots v2 and this is a use-case we should definitely take into account. Would you be willing to share your thoughts on this discussion #454? |
I'm with @BlakeWilliams that this feels like a use case that Slots could/should support. If this doesn't get baked into slots I'd encourage us to think hard about the rendering API and making it "Rails like". I agree with the four points you mention - they seem consistent with ViewComponents strong APIs and cached templates. However, the |
Hi! I remember discussing this with @joelhawksley , and he also said that slots v2 could take this role. Maybe it can, but I have not yet seen how that works. Discussion #454 doesn't mention slot templates, for example. I have posted my further thoughts there. I think we first need to address this bigger point before tackling the one raised by @jonspalmer (although I do agree that |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Hi! Friendly ping to ask about this feature. Do you consider Slots V2 to cover this use case? AFAICT, it could be, but it is rather cumbersome:
All three options also expose the slot to the user, which is not desired. If splitting templates is still a desired feature, I'd like to move this forward, and also give it a nicer interface than |
I agree 2 and 3 are mostly incompatible with this goal (2 could work imo, depending on the use case). I do think option 1 could work though. Your point about the slots being exposed to the user is completely valid, but I wonder if that could be resolved by adding a |
Hi @BlakeWilliams ! I'm going to create an example. Imagine a Table Component: class TableComponent < ApplicationComponent
renders_many :columns, "ColumnComponent"
def initialize(rows:)
@rows = rows
end
class ColumnComponent < ApplicationComponent # This one actually doesn't need to be a component :)
def initialize(attribute:)
@attribute = attribute
end
def render_value model
model.send(@attribute) # for brevity, we don't implement other ways of rendering the value
end
end
end <%# table_component.html.erb %>
<table>
<% @rows.each do |row| %>
<tr>
<% columns.each do |col| %>
<td><%= col.render_value(row) %></td>
<% end %>
</tr>
<% end %>
</table> Now, imagine that I add a bunch of options, and thus creating each row is a bit more complicated. Thus, I want to extract a method to render the row. I can use option 1 above: class RowComponent
def initialize(item:, columns:, **some_more_options)
@item = item
@columns = columns
@options = some_more_options
end
def row_options
# something based on @options
end
end <%# row_component.html.erb %>
<%= content_tag :tr, row_options do %>
<% @columns.each do |col| %>
<td><%= col.render_value(@item) %></td>
<% end %>
<% end %> class TableComponent < ApplicationComponent
renders_many :columns, "ColumnComponent"
# @api private
renders_many :rows, -> (item) do
RowComponent.new(item: item, columns: columns, **some_options)
end
def initialize(rows:)
@rows = rows
end
def before_render
rows(@rows.map{|item| {item: item, columns: columns} })
end
end <%# table_component.html.erb %>
<table>
<% rows.each do |row| %>
<%= row %>
<% end %>
</table> The problem with this solution is that:
|
@fsateler are you still interested in building out this functionality? If so, perhaps opening a PR with an ADR would be a good next step? |
Hi @joelhawksley ! Indeed I might. I have two questions though:
|
c2bbebb
to
a61321b
Compare
Hi! I have rebased the branch and added the ADR. |
lib/view_component/compiler.rb
Outdated
raise ArgumentError, "Arguments already defined for template #{template}" if template_arguments.key?(template) | ||
|
||
template_exists = templates.any? { |t| t[:base_name].split(".").first == template } | ||
raise ArgumentError, "Template does not exist: #{template}" unless template_exists |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking this check should also be the other way around: we should only compile methods for the templates we have described (either via the DSL or the def render_foo
). WDYT?
Hey @fsateler, I'm a latecomer to this PR and discussion, but I hope you'll allow me to state my opinion anyway. I'm concerned rendering partials in components breaks the component abstraction. One of the benefits of view_component is that it avoids many of the problems inherent in traditional Rails views, key among them being the problem of shared state. While there's nothing stopping you from rendering normal Rails view partials inside components today, I'm not sure it's a good idea to bake it into the view_component framework like this. Doing so further encourages using partials that should probably be their own components. There's already a fair amount of confusion around how slots are used, and I fear partials with their own partial methods will add to this confusion. Is there something preventing you from converting your partials into components? |
@camertron I think it is best if you comment directly on the parts of the ADR that don't make sense to you. I don't know what is missing there to answer your question. |
Btw, this merge explicitly is not about invoking unrelated partials. Its about being able to extract some parts of a large method into several smaller methods |
@fsateler ah I used the word "partial" when I meant to say "additional sidecar asset." Correct me if I'm wrong, but a big part of your proposal is providing additional .html.erb files scoped to the component (i.e. sidecar assets) that get rendered from another sidecar asset. Perhaps my earlier comment makes more sense with the right terminology gsubbed in 😅 I'll make additional comments on the ADR 😄 |
619089c
to
7210804
Compare
6e86882
to
a04f004
Compare
79cdef0
to
d0ca761
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! I'll have another round of feedback around docs/ADR once these items are resolved ❤️
docs/guide/templates.md
Outdated
@@ -131,3 +131,74 @@ class MyComponent < ViewComponent::Base | |||
strip_trailing_whitespace(false) | |||
end | |||
``` | |||
|
|||
## Multiple templates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
## Multiple templates | |
## Sub-templates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about "Partial templates" or "Partials"?
Partial templates - usually just called "partials" - are another device for breaking the rendering process into more manageable chunks. With a partial, you can move the code for rendering a particular piece of a response to its own file.
https://guides.rubyonrails.org/layouts_and_rendering.html#using-partials
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe "Sidecar partials" to remove ambiguity?
Or "Component partials" to match the phrasing used in the ADR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the term in theory, but in practice I think it's already "taken" by Rails' usage of it. As this feature is quite different from Rails partials, I'm hesitant to use the term.
9a31362
to
da2c57e
Compare
I have applied (almost) all suggestions. Thanks for the review. I liked the new method name pattern. I did not apply @Spone 's suggestion for the new name in the documentation, but that could be applied easily if there is consensus about that. IMO, the name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another round, I think we're getting pretty close ❤️
54e23a1
to
176d52a
Compare
821ebd1
to
2d31654
Compare
2d31654
to
ea76dad
Compare
Looking at this during office hours today, I think we're in a tough spot. We now have a situation where some ViewComponent templates support variants and some don't, which I think would be quite confusing to consumers. (Although I'm of the opinion generally that variants shouldn't exist at all). This makes me want to lean more on @BlakeWilliams's inline templates proposal. |
Hi!
I don't understand this point. AFAICT, you can have variants in this PR. Perhaps you mean you need to know which variant to render? That may be addressable with something like I proposed earlier: #451 (comment) def render_foo_template **args
case @__vc_variant
when 'some_variant'
render_list_template_some_variant **args
else
render_list_template_default_variant **args
end
end This would allow rendering the current template forcing a variant (via the |
Hi @joelhawksley ! I'm still interested in moving this forward, but I'd need to understand better this objection:
You can have variants in this PR, although they are not very ergonomic. Do you want me to think about improving that? Or is something else you have in mind? |
Reading over this (trying to get the full context of the direction of variants), I
Isn't this exactly the problem lazy enumerators are supposed to solve? |
Not really. Lots of transformations are more memory efficient in lazy enumerators. For example, In this case, however, we are doing a |
@fsateler responding to #387 (comment) here ❤️ Revisiting this with @camertron and @BlakeWilliams, we're wondering if it'd make sense to first land hooks to the compiler that would allow a gem to add this behavior. I don't think it would be too bad to do so. We're still ultimately feeling hesitant to confuse folks' use of Slots, which are already quite complex. I also wonder if we might want to consider a Rails core contribution to add named render helper methods for views such as |
@joelhawksley awesome! if there is anything that I can help with I'm happy to do so. |
Co-authored-by: Rob Sterner <[email protected]> Co-authored-by: Joel Hawksley <[email protected]>
```ruby class TestComponent < ViewComponent::Base template_arguments :list, :multiple # call_list now takes a `multiple` keyword argument def initialize(mode:) @mode = mode end def call case @mode when :list call_list multiple: false when :multilist call_list multiple: true when :summary call_summary end end end ```
ea76dad
to
3b1d674
Compare
Hi @joelhawksley any chance this feature could be released? |
Summary
Fixes #387
This PR builds on the branch by @joelhawksley and @fermion, and adds the ability to specify arguments for the generated functions.
The logic being that one may want to use templates to extract sections, without wanting to build a full component. For example, I could have a Table component, and I would want to have the row definition in a sidecar template. This requires being able to pass in each item for every iteration:
Conceptually, this is very similar to ActionView's usage of
locals:
. However, this way has the following benefits:instance_exec
things in a custom-build binding, further improving performance.Other Information
I'm submitting as a draft, because there are some important API issues that need discussing: