Skip to content
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

Allow passing arguments to a slot block #1810

Closed
RolandStuder opened this issue Jul 28, 2023 · 5 comments
Closed

Allow passing arguments to a slot block #1810

RolandStuder opened this issue Jul 28, 2023 · 5 comments

Comments

@RolandStuder
Copy link

RolandStuder commented Jul 28, 2023

Feature request

In the case of some complex components, I want to create a abstracted API. The example I have is a TableComponent

When I use a component, I can use the component instance in a block like:

<%= render TableComponent.new(rows: @products) do |table|
  <%= table.with_column(label: "Name") { |column|  }
<% end >

The current API does not allow you to iterate through the rows and render all column with the data of said rows by declaring a slot like this. So in order to achieve that I had to create a work around (that I will present later). I wondered if others would find it a worthwhile addition to allow controlling what is passed into the block, so one can do:

<%= render TableComponent.new(rows: @products) do |table|
  <%= table.with_column(label: "Name", &:name)
  <%= table.with_column(label: "Price") { |product| number_to_currency(product.price) }
<% end >

Motivation

I some instances allowing to control what is passed into the slot block when rendering it, can allow for some really nice api, when iterating through collections.

Workaround

You can make the above API work by writing you own methods like:

class TableComponent < ViewComponent::Base
  def initialize(rows:)
    @rows = rows
    @columns = []

  end

  def column(label, &block)
    @columns << Column.new(label, &block)
  end

  def  before_render
     content # to ensure block is called
  end
  
  class Column
    attr_reader :label, :td_block
    def initialize(label, &td_block)
      @label = label
      @td_block = td_block
    end
  end
end

Then in the component template you can do:

  <tr>
    <% columns.each do |column| %>
        <th><%= column.label %>
     <% end %>
  </tr>
  <% @rows.each do |row| %>
    <tr>
      <% @columns.each do |column| %>
        <td>
          <%= view_context.capture(row, &column.td_block) %>
        </td>
      <% end %>
    </tr>
  <% end %>

So this works wonderfully (though it took a while to figure it out).

So I got a solution for me, but wondered whether we might add this functionality to the core library, so it could be used like this:

Proposal

Allow to declare slots like normal

class TableComponent < ViewComponent::Base
  renders_many :columns, TableComponent::Column
  ...
  class Column < ViewComponent::Base
     def initialize(label)
        @label = label
     end

     def title(label)
       content_tag :h2, label
     end
  end
end

In the component template file, allow to pass args, that will be passed to the block upon rendering:

  <% @rows.each do |row| %>
    <tr>
      <% columns.each do |column| %>
        <td>
          <%= column(row) %>
        </td>
      <% end %>
    </tr>
  <% end %>

So in this case you would pass the row data into the block instead of the component, you could still pass the component as needed.

column(column, row)

So you can use it in the view like this:

<%=   TableComponent(rows: @products).new |table| %>
  <%=  table.with_column |column, product| %>
       <%= column.title_style product.style %>
       <%= truncate(product.style) %>
   <% end %>
<% end %>

It would not be hard to adjust the view component code to adjust this. Though I am not sure if this kind of abstraction is what view component is striving for, especially since in this example you see the Column is not really a view component, but only an object to hold the block and params to configure the column, so it is muddying the responsability of the ViewComponent a bit.

ps: I would be happy to make a PR to enable this, I am also fine if this doesn't fit the vision of ViewComponent and the proposal is rejected.

@RolandStuder
Copy link
Author

I tried to have a go at this, but of course I see that it is not as simple as I imagined. In the end it is about the slot being rendered, as its to_s methods is called. So there are two challenges, kinda looks like you have to pass thing into the render_in call… but also the to_s method is being cached, so one would have to remove that cache.

Going deeper into the render_in method it's hard to understand what is going on with the render_template_for method, I really didn't understand at all, what was going on there.

I think the proposed API above is probably not possible, while doing something like column.render_with_params(row) or something like that might be achievable.

@camertron
Copy link
Contributor

camertron commented Aug 22, 2023

Hey @RolandStuder, thanks for bringing this up 😄 Please correct me if I'm wrong, but can't this be done by passing product into the component on initialization?

<%= render(TableComponent.new(product: product)) do |table| %>
  <% table.with_column(label: "Price") { number_to_currency(product.price) } %>
<% end %>

@reeganviljoen
Copy link
Collaborator

@RolandStuder do you think this accurately solves your issue or do you still feel its necessary for an alternative implementation

@RolandStuder
Copy link
Author

@camertron @reeganviljoen No, the point is that I am want to render one slot multiple times with different data. I am rendering a (table) column cell for every entry in @products.

The problem is not that I can't pass in the products to one slot rendering instance, the problem is that a slot can kinda only be used once. I can't do that:

render TableComponent.new(rows: @products) do |table|
    table.with_column(:price) { product.price }
end

The component table_component.html.erb:

<table>
  <tr>
        <%  columns.each do |column| %>
            <th><%= column.label %>
        <% end %>
  </tr>
  <%= @products.each do |product| %>
        <tr>
           <% columns.each do |column| %>
              HERE IS THE PROBLEM
              <% column # can't pass anything here to the column to render with that specific data %>
           <% end %>
        </tr>
     <% end %>
</table>

On the other hand I also cannot do this:

render TableComponent.new do |table|
    @products.each do |product|
       table.with_column(:price) { product.price }
    end
end

As with this, I can't render the column headings, as there is a column slot for every cell in the table, also it forces me away from the API I wanted for our component.

I think the abstraction I want is just a bit outside of what ViewComponent is indeted to support. When I researched the topic I was surprised to find out there was basically no one suggesting such a column focused approach. So I think it just abstracts a bit more than would people usually abstract in a VC.

I think it's fine to just close this issue. I have a working workaround. The fact that is is hard to communicate and that it doesn't work like a regular slot, is probably a "smell" to say, "you shouldn't do this with ViewComponent" 😅

I described my approach also here, not sure if I communicated it better there.

@camertron
Copy link
Contributor

Hey @RolandStuder thanks for that additional information. Reading through your blog post provided the context I was missing, specifically this line:

personally, I prefer to think of tables in terms of columns

Ahhhhh ok now everything makes sense! IMO, the fundamental problem you're running into here isn't ViewComponent, or even ERB - it's HTML itself. HTML definitely thinks of tables from a row perspective, and requires you to construct them row by row. ViewComponent would have to somehow maintain "cursors" at multiple points in the output buffer to write a table by columns, which I don't think is possible given how the Rails view layer is architected. In fact, I don't know any web framework that has this capability.

The approach you shared here and in the blog post, that uses plain 'ol Ruby objects (POROs) instead of components, seems like a great way to solve the problem. Slots are not designed to be rendered multiple times, so we are unlikely to accept a change that would allow them or their #to_s methods to accept arguments.

In thinking about the problem from this fresh perspective, I've come up with an alternative approach that uses ViewComponents and includes limited trickery.

Here's how the API looks:

<%# app/views/something/index.html.erb %>
<%= render(TableComponent.new) do |table| %>
  <% table.with_column(title: "Col1") do |column| %>
    <% column.with_item { "val1" } %>
    <% column.with_item { "val2" } %>
    <% column.with_item { "val3" } %>
  <% end %>
  <% table.with_column(title: "Col2") do |column| %>
    <% column.with_item { "val1" } %>
    <% column.with_item { "val2" } %>
    <% column.with_item { "val3" } %>
  <% end %>
<% end %>

There are three components at play here, TableComponent, ColumnComponent, and ItemComponent.

TableComponent

# app/components/table_component.rb

class TableComponent < ViewComponent::Base
  renders_many :columns, ColumnComponent

  private

  def num_rows
    columns.map { |col| col.items.size }.max
  end
end
<%# app/components/table_component.html.erb %>

<table>
  <% columns.each do |column| %>
    <%= column %>
  <% end %>
  <% num_rows.times do |i| %>
    <tr>
      <% columns.each do |column| %>
        <%= column.items[i] %>
      <% end %>
    </tr>
  <% end %>
</table>

ColumnComponent

# app/views/column_component.rb

class ColumnComponent < ViewComponent::Base
  renders_many :items, ItemComponent

  def initialize(title:)
    @title = title
  end
end
<%# app/views/column_component.html.erb %>

<th><%= @title %></th>

ItemComponent

# app/views/item_component.html.erb

class ItemComponent < ViewComponent::Base
end
<%# app/views/item_component.html.erb %>

<td><%= content %></td>

This is probably what I would do if I wanted to create a column-focused table component.

Since you already have a viable workaround, and because we are unlikely to support rendering slots with arguments, I'm going to close this issue. Please feel free to reopen if necessary 😄

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

No branches or pull requests

3 participants