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

Implement Slots as potential successor to Content Areas #348

Merged
merged 16 commits into from
Jun 30, 2020
Merged

Conversation

joelhawksley
Copy link
Member

@joelhawksley joelhawksley commented May 20, 2020

This PR introduces Slots, an evolution of the Content Areas feature in ViewComponent.

Problem statement

For the sake of explanation, I am going to use the Box component from Primer CSS, GitHub's design system.

The Box component can have a single header, body, and footer, along with multiple rows.

An example Box might look something like:

<div class="Box">
  <div class="Box-header">
    Box header
  </div>
  <div class="Box-body">
    <strong>Box body</strong>
  </div>
  <ul>
    <li class="Box-row">
      Box row one
    </li>
    <li class="Box-row">
      Box row two
    </li>
    <li class="Box-row">
      Box row three
    </li>
  </ul>
  <div class="Box-footer">
    Box footer
  </div>
</div>

With Content Areas, this could be implemented as:

# box_component.rb

class BoxComponent < ViewComponent::Base
  content_areas :header, :body, :footer
  
  def initialize(rows: [])
    @rows = rows
  end
end

# box_component.html.erb

<div class="Box">
  <% if header %>
    <div class="Box-header">
      <%= header %>
    </div>
  <% end %>
  <% if body %>
    <div class="Box-body">
      <%= body %>
    </div>
  <% end %>
  <% if rows.any? %>
    <ul>
      <% rows.each do |row| %>
        <li class="Box-row <%= row[:class_names] %>">
          <%= row[:body] %>
        </li>
      <% end %>
    </ul>
  <% end %>
  <% if footer %>
    <div class="Box-footer">
      <%= footer %>
    </div>
  <% end %>
</div>

There are some shortcomings to this approach:

Collections

Having to pass to pass in rows to BoxComponent has proven awkward. Such an approach forces us to implement features such as required and default arguments ourselves, patterns Ruby gives us for free in object initializers!

This issue has been discussed in glorious length.

Area-specific arguments

With content areas, any area-specific logic or configuration generally ends up being passed into the component initializer.

For example, if we wanted to allow for class names to be set on each content area, our initializer might look something like:

# box_component.rb

class BoxComponent < ViewComponent::Base
  content_areas :header, :body, :footer
  
  def initialize(
    rows: [], 
    header_class_names: "", 
    body_class_names: "",
    footer_class_names: ""
  )
    @rows = rows
    # ...
  end
end

And if we needed to set different class names on each row, we would need to write code to handle default values and required arguments ourselves.

For more context, see @rosendi's issue.

Existing solutions

@andrewmcodes and I implemented a method for passing arguments to Content Areas in #322, assigning the arguments to a hash named after the content area.

@jonspalmer also implemented a solution here: #323, using a block-based approach.

@jensljungblad's elemental_components has implementations of both area-specific arguments and collection areas, and has been held up as a desirable API.

Proposed solution

I propose that we move to a new API that I'm calling Slots.

Why the name “Slots”?

After shipping Content Areas, we received feedback that the feature was very similar to Slots in Vue and in Web Components.

As we will likely need to make breaking changes to the Content Areas API, I figured this was a good time to align the name of the feature with the greater ecosystem.

Show me the code already!

Here's what our BoxComponent might look like with Slots:

# box_component.rb

class BoxComponent < ViewComponent::Base
  with_slot :body, :footer
  with_slot :header, class_name: "Header"
  with_slot :row, class_name: "Row", collection: true
 
  class Header < ViewComponent::Slot
    def initialize(class_names: "")
      @class_names = class_names
    end
    
    def class_names
      "Box-header #{@class_names}"
    end
  end
  
  class Row < ViewComponent::CollectionSlot
    def initialize(theme: :gray)
      @theme = theme
    end
    
    def theme_class_name
      case @theme
      when :gray
        "Box-row--gray"
      when :hover_gray
        "Box-row--hover-gray"
      when :yellow
        "Box-row--yellow"
      when :blue
        "Box-row--blue"
      when :hover_blue
        "Box-row--hover-blue"
      else
        "Box-row--gray"
      end
    end  
  end
end

# box_component.html.erb

<div class="Box">
  <% if header %>
    <div class="<%= header.class_names %>">
      <%= header.content %>
    </div>
  <% end %>
  <% if body %>
    <div class="Box-body">
      <%= body.content %>
    </div>
  <% end %>
  <% if rows.any? %>
    <ul>
      <% rows.each do |row| %>
        <li class="Box-row <%= row.theme_class_name %>">
          <%= row.content %>
        </li>
      <% end %>
    </ul>
  <% end %>
  <% if footer %>
    <div class="Box-footer">
      <%= footer %>
    </div>
  <% end %>
</div>

# index.html.erb

<%= render(BoxComponent.new) do |component|
  <% component.slot(:header, class_names: "my-class-name") do %>
    This is my header!
  <% end %>
  <% component.slot(:body) do %>
    This is the body.
  <% end %>
  <% component.slot(:row) do %>
    Row one
  <% end %>
  <% component.slot(:row, theme: :yellow) do %>
    Yellow row
  <% end %>
  <% component.slot(:footer) do %>
    This is the footer.
  <% end %>
<% end %>

Slot types

Slots exist in two forms: normal slots and collection slots.

Normal slots (:header, :body, and :footer above) can be rendered once per component. They expose an accessor with the name of the slot (#header, #body, and footer) that returns an instance of BoxComponent::Header, etc.

Collection slots (:row above) can be rendered multiple times. They expose an accessor with the pluralized name of the slot (#rows), which is an Array of BoxComponent::Item instances.

Defining slots

Slots are defined by the with_slot macro:

with_slot :header

To define a collection slot, add collection: true:

with_slot :row, collection: true

To define a slot with a custom class, pass class_name:

with_slot :body, class_name: 'BodySlot

Slot classes should be subclasses of ViewComponent::Slot.

Next steps

My intention with this PR is to codify a new API to ship as an experimental feature, with the goal of deprecating Content Areas if this approach proves successful.

I can't thank y'all enough for all of the insightful discussion that has happened around this initiative so far ❤️

Co-authored-by: Jon Palmer jonspalmer@gmail.com
Co-authored-by: Brian Bugh brainbag@gmail.com
Co-authored-by: Jens Ljungblad jens.ljungblad@gmail.com

cc @bbugh

@joelhawksley joelhawksley requested a review from a team May 20, 2020 19:13
@joelhawksley joelhawksley force-pushed the slots branch 5 times, most recently from 1479927 to b418b1d Compare May 20, 2020 19:39
@joelhawksley joelhawksley requested a review from jonspalmer May 20, 2020 19:44
Copy link
Contributor

@jonspalmer jonspalmer left a comment

Choose a reason for hiding this comment

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

I love this. 💯 🚀 🥳. The API for defining these slots is naturally Ruby args and blocks and natural to extend.

A few comments around some of the internals and options to make it extendable by consumers.

For what it's worth #323 is an orthogonal concern and we could add a similar set of functionality to this solution if it was important.

Amazing work!

lib/view_component/base.rb Outdated Show resolved Hide resolved
lib/view_component/base.rb Outdated Show resolved Hide resolved
lib/view_component/base.rb Outdated Show resolved Hide resolved
lib/view_component/base.rb Outdated Show resolved Hide resolved
lib/view_component/base.rb Outdated Show resolved Hide resolved
lib/view_component/slotable.rb Outdated Show resolved Hide resolved
lib/view_component/base.rb Outdated Show resolved Hide resolved
@bbugh
Copy link
Contributor

bbugh commented May 21, 2020

I am thrilled ❤️. The API looks fantastic, very nice work. I love that it's standard Ruby syntax all around and very easy to understand at a glance.

One thing I noticed while giving it a quick look-over: it seems like there's undefined behavior when you have both a with_slots and a Slot class with arguments, with the same name:

class BoxComponent < ViewComponent::Base
  with_slots :header # takes 0 arguments
  
  class Header < ViewComponent::Slot # now it takes 1 optional argument
    def initialize(class_names: "")
      @class_names = class_names
    end
  end

  with_slots :header # now it takes 0 arguments again

Since with_slots is creating a Header class internally, Ruby will overwrite the initialize with a different arity, but depending on the order of the class definition and with_slots it might or might not have arguments (Schrödinger's arity?). Maybe this could be a warning or error - "Hey you already defined a slot class, don't do it manually 🐛"

Also, I wonder if maybe the subclass should have a domain suffix like HeaderSlot to avoid any possible collisions? The only use case I can think of off the top of my head is extracting the slot object to a separate file (header_slot.rb vs header.rb), but it does mirror The Rails Way of suffixing everything except models.

Thanks for putting this together @joelhawksley. I'm excited to get some time to test it out soon!

Copy link
Contributor

@bbugh bbugh left a comment

Choose a reason for hiding this comment

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

The slot/slot_names bug is a blocker.

lib/view_component/base.rb Outdated Show resolved Hide resolved
lib/view_component/base.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@jensljungblad jensljungblad left a comment

Choose a reason for hiding this comment

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

I like the idea of using classes as much as possible!

Some questions:

Is there precedence in Rails for defining methods automatically based on defined subclasses in the class? The fact that writing class Header < ViewComponent::Slot; end would generate methods on the class seems unexpected. Then again, I guess this is Ruby.. It's a cool implementation, but does it violate the law of least surprise? I'd be interested in hearing if this is a pattern used elsewhere.

Two alternatives come to mind:

  1. Always require using with_slots (should this be with_slot btw? Similar to attr_reader family which is always singular) but pass the class constant. Something like:
class BoxComponent
  class Header < Slot; end
  with_slot Header
end

That would make the API consistent (you always use with_slot) but you can pass a symbol for an anonymous class or your own class if you want attributes etc.

  1. Pass a block to with_slot with the anonymous class definition:
class BoxComponent
  with_slot :header do
    def initialize(class_names:); end
  end
end

Would also make the API consistent (always use with_slot) and this does have precedence in Rails, specifically ActiveRecord which allows you to pass a block to association declarations and within the block define methods that will be mixed into the relation.

It has the downside of looking more like a DSL (I do like dealing with classes, personally) and the other downside of the class being anonymous, meaning you cannot reference it as easily, if you need to do that.

@bbugh
Copy link
Contributor

bbugh commented May 21, 2020

Hey @jensljungblad, really appreciate you chiming in on this since you already have domain knowledge about the tribulations of designing a Rails component library!

Is there precedence in Rails for defining methods automatically based on defined subclasses in the class?

Off the top of my head, the only place I know for sure in Rails that generates dynamic classes is has_and_belongs_to_many joins. I think RSpec is a good parallel in this case, they generate dynamic classes for each nested level of their DSL, e.g.:

describe 'top level test' do
  self # => RSpec::ExampleGroups::TopLevelTest
  
  describe 'validations' do 
	self # => RSpec::ExampleGroups::TopLevelTest::Validations
  end
end

@jensljungblad
Copy link
Contributor

jensljungblad commented May 21, 2020

Off the top of my head, the only place I know for sure in Rails that generates dynamic classes is has_and_belongs_to_many joins. I think RSpec is a good parallel in this case, they generate dynamic classes for each nested level of their DSL, e.g.:

describe 'top level test' do
  self # => RSpec::ExampleGroups::TopLevelTest
  
  describe 'validations' do 
	self # => RSpec::ExampleGroups::TopLevelTest::Validations
  end
end

That's a DSL though! I would expect that to generate methods.

I specifically meant generating methods from an explicit class declaration. Like how writing class Header < ViewComponent::Slot; end would actually generate methods on the parent class. In this case a #header method. That's what I meant feels pretty unexpected.

@jensljungblad
Copy link
Contributor

jensljungblad commented May 21, 2020

Another potential benefit with passing the class to with_slot could be re-use of the same class for multiple slots:

class BoxComponent
  with_slot :header, Section
  with_slot :footer, Section

  class Section; end
end

That would mean a de-coupling the method name (i.e. #header) and the class name.

# generate a header method pointing to an anonymous class
with_slot :header 

# generate a header method pointing to a custom class
with_slot :header, Header

# or perhaps more verbosely
with_slot :header, class: Header

@jonspalmer
Copy link
Contributor

Forgive me if this is a tangent but I wonder if we need the Slot classes. What if they could be any class and we do away with the auto registering?

In particular that would allow the slots to be ViewComponents themselves. Imagine your box component example like this:

# row_component.rb

  class RowComponent < ViewComponent::Base
    def initialize(theme: :gray)
      @theme = theme
    end
    
    def theme_class_name
      case @theme
      when :gray
        "Box-row--gray"
      when :hover_gray
        "Box-row--hover-gray"
      when :yellow
        "Box-row--yellow"
      when :blue
        "Box-row--blue"
      when :hover_blue
        "Box-row--hover-blue"
      else
        "Box-row--gray"
      end
    end  
  end

# row_component.html.erb

<li class="Box-row <%= row.theme_class_name %>">
  <%= row.content %>
</li>

# box_component.rb

class BoxComponent < ViewComponent::Base
  with_slots :body, :footer
  
  class Header 
    def initialize(class_names: "")
      @class_names = class_names
    end
    
    def class_names
      "Box-header #{@class_names}"
    end
  end
  
  with_slots Header
  with_collection_slots Row
end

# box_component.html.erb

<div class="Box">
  <% if header %>
    <div class="<%= header.class_names %>">
      <%= header.content %>
    </div>
  <% end %>
  <% if body %>
    <div class="Box-body">
      <%= body.content %>
    </div>
  <% end %>
  <% if rows.any? %>
    <ul>
      <% rows.each do |row| %>
        <%= render row %>
      <% end %>
    </ul>
  <% end %>
  <% if footer %>
    <div class="Box-footer">
      <%= footer %>
    </div>
  <% end %>
</div>

consumers could do things like this to swap out the Row implementation

class MyCustomBoxComponent < BoxComponent
  with_collection_slots row:, MySpecialRowComponent
end

@jonspalmer
Copy link
Contributor

Another thought on the API.

Do we need with_collection_slots :row or could it simply be with_slots :row, collection: true ?

@jensljungblad
Copy link
Contributor

jensljungblad commented May 22, 2020

So the requirement is to allow anonymous and user defined slot classes. I believe these are the suggestions:

1. Auto-register classes

class Box
  with_slots :header, :subheader # anonymous slot
  with_collection_slots :items # anonymous collection slot

  class Footer < Slot; end # user defined slot
  class Row < CollectionSlot; end #user defined collection slot
end

Benefits:

  • Can define multiple anonymous slots on one line
  • Can define user defined slot with one line, rather than two
  • There are probably more :)

Drawbacks:

  • Unexpected behavior that defining a class leads to methods being generated (subjective?)
  • Two ways of defining slots (four if you count collection slots)
  • Implementation needs to eval stuff

2. Use macro to point to user defined slot class

Infer slot name from class name.

class Box
  with_slots :header, :subheader, Footer
  with_collection_slots :items, Row

  class Footer; end
  class Row; end
end

Benefits:

  • One way of defining slots (with_slots)
  • Can define multiple slots on one line
  • Slot classes can inherit from anything, no eval implementation etc.

Drawbacks:

  • Need two lines to define user defined slots (with_slots + class Footer)

3. Use macro to point to user defined slot class but without inferring name

No inferring of slot name from class name.

class Box
  with_slot :header
  with_slot :subheader
  with_slot :footer, Footer
  with_collection_slot :item, Item
  with_collection_slot :row, Item

  class Footer; end
  class Item; end
end

Benefits:

  • One way of defining slots (with_slot)
  • Can re-use classes between slots (the class name is de-coupled from the generated method names)
  • Allows for other options, such as collection: true, if desired

Drawbacks:

  • More lines needed

3.1 Use macro to point to user defined slot class but without inferring name, with collection argument

Variation on 3 with multiple arguments to with_slot.

class Box
  with_slot :header
  with_slot :subheader
  with_slot :footer, class: Footer
  with_slot :item, class: Item, collection: true
  with_slot :row, class: Item, collection: true

  class Footer; end
  class Item; end
end

Benefits/Drawbacks:

  • Similar to other macros in Rails (subjective if that's a good thing!)
  • Always same method used for defining slots (also subjective)

@jensljungblad
Copy link
Contributor

In particular that would allow the slots to be ViewComponents themselves

@jonspalmer What benefits would that bring? It's what I ended up with myself in https://github.com/jensljungblad/elemental_components but that was mostly because I wanted to re-use the attributes API I had already defined for my component class, which doesn't seem to be an issue here, unless nesting is desired (i.e. wanting to use with_slot in your slot class) but I'm not sure that's the case either!

(I think it's good to leave that up to the user though, so if the class can be anything and doesn't need to inherit from a specific superclass, that's nice)

@jensljungblad
Copy link
Contributor

I experimented a little myself in #354.

I've got one general question:

  1. What do you think the expected behavior should be for calling on slots (regular and collection slots) when they haven't been populated yet? To me it makes sense that calling #tabs would return an empty array, and not nil, if component.slot(:tab) hasn't been called. For regular slots, nil seems reasonable? That way you could validate their existence etc.

As for my experiment, two things that would need addressing:

  1. Might make sense to make class_name or however that argument would be passed a string instead of a constant, similar to ActiveRecord, so that the classes could be defined at the bottom of the file, marked private etc.
  2. Perhaps the classes could be picked up automatically as well, similar to ActiveRecord. That is, a with_slot :footer would look for a SlotsComponent::Footer constant and use that as the class if none was passed.
  3. I kept the slots inheriting from a base Slot class only because of the content accessor. That could be dealt with in some other way so users can inherit from whatever base class they want.

@jonspalmer
Copy link
Contributor

@jensljungblad Benefit of allowing ViewComponents as the slot class would be that it would allow "bringing your own view" for the rendering of the slot and it would allow various options to swap out that implementation. I'm thinking about how you can vary the piece of things like React Select by passing it component classes.

What do you think the expected behavior should be for calling on slots
I definitely think collection slots should be an empty array.

similar to ActiveRecord,
What are the active record examples you're referencing here?

I'm a big fan of the with_slot you describe in 3.1. This signature would be sweet:

def self.with_slot(*slot_names, class: ViewComponent::Slot, collection: false)

In addition I'd add the wrinkle that slot_names would take symbols or classes. If you pass a class then it results in a slot with the name of that class. That would allow these combinations:

  # These all results in slots for :header, :body, :footer but different classes used
  with_slot :header, :body, :footer
  with_slot :header, :body, :footer, class: CustomSlot
  with_slot Header, Body, Footer

  with_slot :subheader, class: HeaderSlot
  
  # These all results in an `items` collection slot but different classes used
  with_slot :item, collection: true
  with_slot Item, collection: true
  with_slot :item, class: Row, collection: true

@jensljungblad
Copy link
Contributor

@jensljungblad Benefit of allowing ViewComponents as the slot class would be that it would allow "bringing your own view" for the rendering of the slot and it would allow various options to swap out that implementation. I'm thinking about how you can vary the piece of things like React Select by passing it component classes.

👍

What are the active record examples you're referencing here?

It actually relates to your suggestions.

Consider:

class Box < Component
  with_slot Header
  class Header; end
end

This wont work, since the Header constant is referenced before being defined. You'd need to do:

class Box < Component
  class Header; end
  with_slot Header
end

Maybe that's fine, but personally I'd probably prefer making these classes private and keep them at the bottom of the file. This is one reason ActiveRecord makes use of strings referencing class names instead of constants, for associations:

has_many :things, class_name: "Thing"

That way the constant can be resolved at a later time.

If this should be a consideration at all, I don't think with_slot "Header" makes that much sense. Might as well go with with_slot :header and resolve the class automatically if it exists?

Something like:

class Box < Component
  with_slot :header, :body, :footer # all anonymous
end

class Box < Component
  with_slot :header, :body, :footer

  # will be inferred and used by the :header slot, in a way similar to how AR infer associated
  # models where something like belongs_to :thing would resolve to a Thing class
  class Header; end associated 
end

class Box < Component
  with_slot :header, :body, :footer, class_name: "CustomSlot"

  # will be used by header, body and footer slots. so the class_name would be useful for sharing
  # the same slot class between slots.
  class CustomSlot; end associated 
end

@bbugh
Copy link
Contributor

bbugh commented May 25, 2020

I like the API where each line defines a slot(s) and can take options. It looks like validates in Rails and will probably feel familiar to people. Imagining something like a card header/footer that shares the same slot class:

with_slot :header, :footer, class_name: "EndCap" 
with_slot :items, collection: true

On the other hand, I think it might be weird if it's inferred in some cases but not others:

# is this inferred as class Header and Footer
with_slot :header, :footer

# but this is both EndCap?
with_slot :header, :footer, class_name: "EndCap"

Maybe it makes more sense for each line to be its own definition. I mean, realistically, even complex reusable components like modals are going to max out at 4-6 slots or so.


@jensljungblad: I experimented a little myself in #354.

I think this is a realistic evolution based on some of what we're discussing here. How would you see this progressing?


@jensljungblad What do you think the expected behavior should be for calling on slots (regular and collection slots) when they haven't been populated yet? To me it makes sense that calling #tabs would return an empty array, and not nil, if component.slot(:tab) hasn't been called. For regular slots, nil seems reasonable? That way you could validate their existence etc.

We discussed this in the pull request review here, the gist is that I (and @jonspalmer at the time) agree that nil for a regular slot and [] for a collection slot makes sense as defaults.


@jonspalmer: In particular that would allow the slots to be ViewComponents themselves.

I believe the reasons raised against this idea before was that ViewComponents require templates and can be created by render. I think that while their input API is similar, a Slot is just a lightweight domain-specific value object, while a Component is a heavier-weight behavior object. A slot is also something that has no contextual value outside of the parent component, while components can be composed to build larger things. I'm personally in favor of keeping Slot and Component separated.


@jensljungblad: If this should be a consideration at all, I don't think with_slot "Header" makes that much sense. Might as well go with with_slot :header and resolve the class automatically if it exists?

Symbols do not allow easy namespace resolution, which is the usual reason for also allowing explicit class names, in addition to taking in string names to avoid load order issues. :header could be header, but how do you specify Nested::Header?

@jensljungblad
Copy link
Contributor

Here's an example of resolving the slot classes in the way I mentioned above: 09779e1

@jensljungblad
Copy link
Contributor

jensljungblad commented May 25, 2020

Symbols do not allow easy namespace resolution, which is the usual reason for also allowing explicit class names, in addition to taking in string names to avoid load order issues. :header could be header, but how do you specify Nested::Header?

Yes to be clear I'd be in favor of something specifying the class name explicitly, but as a string to avoid load order issues. That makes namespace resolution easy, since you'd just do class_name: "Nested::Header" etc. I was saying something like with_slot :title, Subtitle doesn't work (load order issues) and with_slot :title, "Subtitle" is kind of unclear (what is the string?) so better then to be explicit with class_name, imo.

As for auto-resolving to an existing class if it exists, I don't know yet if it makes sense. Just thinking aloud :) I pushed a commit here that demonstrates it: 09779e1.

@jonspalmer
Copy link
Contributor

I can see the letting slots be view_components might be confusing. Let's drop that for now until/if we have some concrete usecases to design around.

Maybe it makes more sense for each line to be its own definition. I mean, realistically, even complex reusable components like modals are going to max out at 4-6 slots or so.

@bbugh I'm coming around to that idea too.

@jensljungblad I like the idea of using strings to define the classes. One thing to consider is that in AR there are always classes. On the other hand we imagine simple cases where you should be able to do with_slot :header and get the default "Slot" class. You certainly wouldn't want the surprise of something weird happening if you happened to have a Header class in scope for some non-slot use.

Maybe for terseness we make all of these the resolve a slot with name `header?

with_slot :header, class: "Header"
with_slot class: "Header" # infer the slot_name from the class is no slot_name is provided
with_slot class: "HeaderSlot" # by convention chomp "Slot" off the classname when inferring the slot_name

@joelhawksley
Copy link
Member Author

Maybe it makes more sense for each line to be its own definition.

I agree 100%, if we are going to take the API in the direction that is being suggested here.

I agree that nil and [] are good empty states.

I believe the reasons raised against this idea before was that ViewComponents require templates and can be created by render. I think that while their input API is similar, a Slot is just a lightweight domain-specific value object, while a Component is a heavier-weight behavior object. A slot is also something that has no contextual value outside of the parent component, while components can be composed to build larger things. I'm personally in favor of keeping Slot and Component separated.

Agreed.

@jensljungblad
Copy link
Contributor

jensljungblad commented May 27, 2020

Maybe it makes more sense for each line to be its own definition.

I agree 100%, if we are going to take the API in the direction that is being suggested here.

Depends on the amount of options and how much sense it would make to group several slots on one line I suppose. I could see it going either way! While I typically prefer keeping validations on separate lines, it's nice that validates accommodates both styles.

@jonspalmer One thing to consider is that in AR there are always classes. You certainly wouldn't want the surprise of something weird happening if you happened to have a Header class in scope for some non-slot use.

Hm, good point. I guess the requirements are:

  1. Keep it as simple as possible to create anonymous slots, and to create slots backed by a class.
  2. Specifying a shared slot class isn't as common, so could be more verbose.

And some options:

  1. Do not auto-resolve any classes. Always require specifying class if you want to provide your own
with_slot :header, class_name: "Header"
class Header; end
  1. Do auto-resolve classes, and ignore the fact that it might pick up a class not intended for slot-use (i.e. count on this being very uncommon)
with_slot :header
class Header; end
  1. Do auto-resolve classes, but add a convention that slot classes must have a Slot-suffix, making collisions very unlikely:
with_slot :header
class HeaderSlot; end
  1. Make option 1) a bit less verbose by using blocks (or lambdas)
with_slot(:header) { Header }
class Header; end
with_slot :header, -> { Header }
class Header; end

(Using a block sidesteps the load order issue that strings solve. It also avoids any namespace resolution issues you could have with strings. API-wise a bit odd perhaps. Looks more like something from the Ruby standard library than Rails?)

@jonspalmer The with_slot class: "Header" example would only make sense if we go for a single with_slot line per slot, right?

@jensljungblad
Copy link
Contributor

Here's a commit demonstrating option 4) in the previous comment: 614a4b8.

@jensljungblad
Copy link
Contributor

@jonspalmer

with_slot class: "Header" # infer the slot_name from the class is no slot_name is provided`

I was going to say, what if the class is namespaced, as in with_slot class: "Namespaced::Header, what should the slot name be then? But that led to another thought:

Why would you ever have namespaced slot classes? If you extract a slot from a component, isn't that indicative of that being another component instead of a slot? Quoting from BEM again:

[An element is] a part of a block that has no standalone meaning and is semantically tied to its block.

Copy link
Contributor

@BlakeWilliams BlakeWilliams left a comment

Choose a reason for hiding this comment

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

Added a few small comments, but overall I really like this idea and think this looks good.

I think the largest comment I had was around slots being a DSL instead of being class based. I think that could be worth exploring but I don't think it's a blocker.

with_slot :header, class_name: "Header"
with_slot :row, collection: true, class_name: "Row"

class Header < ViewComponent::Slot
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it would be possible to have slots still inherit from ViewComponent::Base?

If we could do that, I think it'd be one less thing to remember and extracting full components is easier since you no longer have to change what it inherits from.

Copy link
Member Author

@joelhawksley joelhawksley Jun 16, 2020

Choose a reason for hiding this comment

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

I could see that being possible, but my concern with that approach would be that it might imply that components could be used as Slots, when a Slot is just a simple accessor object.

I do wonder if there's a simpler way to express the Slot interface though, as all we really care about is that the object has a content accessor.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't help scratch the itch for mr that slots could themselves be ViewComponents. I've mentioned this before and I don't yet have the concrete uses cases (though i have my suspicions) so I'm prepared to let this go for a future release.

But here is my thinking. If you stripped out some of the magic around class_name and generating classes we're providing in with_slot you could imagine that you had to use slots like this:

<% component.slot(:header, HeaderSlot.new(class_names: "my-class-name")) do %>
    This is my header!
  <% end %>

the requirement on HeaderSlot being that it provide a content property to put the captured block in.

Under that light the component.slot is looking an awful lot like a render call on a ViewComponent.

Now imagine the guts of the slot method was something like this:

    slot = slots[slot_name]

    if args.is_a?(Hash)
      # The class name of the Slot, such as Header
      slot_class = self.class.const_get(slot[:class_name])

      # Instantiate Slot class, accommodating Slots that don't accept arguments
      slot_instance = args.present? ? slot_class.new(args) : slot_class.new
    else
      # args were really a class
      slot_instance = args
    end

    if slot_instance.responds_to(:render_in)
      # RenderInSlotWrapper exposes a `content` property that is the result of `render_in`
      # I'd need to think it through more but I think we need some sort of wrapper that 'captures' the entirety of the rendered ViewComponent
      slot_instance = RenderInSlotWrapper.new(slot_instance).render_in(view_context, &block) 
    else
      # Capture block and assign to slot_instance#content
      slot_instance.content = view_context.capture(&block) if block_given?
    end

My spidey senses says there is something power here.

Copy link
Contributor

@jensljungblad jensljungblad Jun 21, 2020

Choose a reason for hiding this comment

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

@jonspalmer I disagree with the following though:

<% component.slot(:header, HeaderSlot.new(class_names: "my-class-name")) do %>
  This is my header!
<% end %>

If the HeaderSlot is publicly accessible like that, I wouldn't view it as a slot anymore. Slots are internal to the component and shouldn't be usable outside of the component, imo. You can of course put a component in a slot, which would be a very common use case of component composition:

<%= render(MediaObjectComponent.new) do |media_object| %>
  <% media_object.slot(:left) do %>
    <%= render(AvatarComponent.new(image: comment.image)) %>
  <% end %>
  <% media_object.slot(:right) do %>
    <%= comment.body %>
  <% end %>
<% end %>

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with the pattern of using a component in a slot. I believe that will be powerful enough for many uses cases.

However, I don't really see the 'internal' piece of a component quite the same way. I see Slots as defining an interface between the component and the slots it uses. Its an Interface not a concrete implementation. As such a ViewComponent leveraging slots comes with so slot classes. IMO it would be powerful allow those classes to be extended. Its let about using those Slot classes outside the component its that I can extend them (subclassing most likely) to be used in the component.

For example consider the Row example from the docs

class Row < ViewComponent::Slot
    def initialize(theme: :gray)
      @theme = theme
    end

    def theme_class_name
      case @theme
      when :gray
        "Box-row--gray"
      when :hover_gray
        "Box-row--hover-gray"
      when :yellow
        "Box-row--yellow"
      when :blue
        "Box-row--blue"
      when :hover_blue
        "Box-row--hover-blue"
      else
        "Box-row--gray"
      end
    end
  end

The interface that the component relies on is that Row have a content getter/setter and it has a getter for theme_class_name.

No suppose I want to use this component but add some other classes to the header based on logic special to my use case. I'd do this:

class MySpecialRow < Row

  def initialize(theme: :gray, special: :foo)
    super(theme)
    @special = special
  end

  def theme_class_name
     special_classes = some_other_logic(special)
     "#{super} #{special_classes}
  end
end

and then in the component usage

  <% component.slot(:row, MySpecialRow.new(theme: :hover_blue, special: :make_some_magic) do %>
    Special row
  <% end %>

Copy link
Contributor

@jensljungblad jensljungblad Jun 27, 2020

Choose a reason for hiding this comment

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

Hm, couldn't you use composition instead? I'm just thinking composition is generally a better pattern in the view layer than overriding/extending. It's the only way you can build components in React, and it works pretty well there. But I admit I have a hard time seeing all the possibilities of 3rd party components and how people would want to extend them.

I guess to phrase it differently, let's say there's a 3rd party library/gem with Twitter bootstrap components built with view_component. Would you really want to modify them, or would you rather wrap them (composition) with your own components that hold the specialized behavior?

lib/view_component/base.rb Outdated Show resolved Hide resolved
# collection: true|false,
# class_name: "Header" # class name string, used to instantiate Slot
# )
def with_slot(*slot_names, collection: false, class_name: nil)
Copy link
Contributor

@BlakeWilliams BlakeWilliams Jun 16, 2020

Choose a reason for hiding this comment

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

The class_name argument here actually confused me for a good minute or two since I was in the mindset of "this is for the view" and thought it was like HTML class="foo bar" and not a Ruby class name.

This almost makes me think that with_slot might be more clear as a DSL instead of a method that associates with another class?

eg:

with_slot :header, collection: false do |arg1, arg2, arg3|
end

I think that may also have the added benefit of removing nested classes in components but would also keep all slot logic in a single place in the component.

That's probably a huge change and has its own challenges, but I thought it was worth bringing up the confusion I ran into here.

edit: I just saw the main discussion about the DSL style slots. I think there are downsides, but I also think the API having limitations could also be a pro instead of a con and would help clarify when to use a component vs when to use a slot.

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 I share your confusion. The class_name API is meant to mimic ActiveRecord: https://guides.rubyonrails.org/association_basics.html#options-for-belongs-to-class-name.

As you mentioned, there has been a lot of debates about the merits of a DSL approach vs. this Class-based approach. I could honestly go either way at this point, but I keep coming back to the fact that Slots are, in my mind, not meant to stand on their own conceptually, let alone be referenced/exist outside the scope of a single component.

@bbugh you seemed to have the strongest feelings about having the Class-based implementation. What's your feeling at this point?

Copy link
Contributor

Choose a reason for hiding this comment

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

not meant to stand on their own conceptually, let alone be referenced/exist outside the scope of a single component.

That makes sense to me, and I think builds on your above comment about slots just having a simple content accessor.

I wonder if both of those points help make a stronger case for the DSL API over Ruby objects?

Personally I think the DSL style helps reinforce "this is not a component, it's just a smaller, isolated part of a component with its own encapsulated behavior and responsibilities".

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that DSLs are nice, but I think it is very important not to use a DSL here and allow class_name to be set. I explained my reasoning here, let me know if you have more questions. #348 (comment)

In short, using the DSL in this way means that the API must choose to either allow component subclasses to replace a slot OR to extend a slot, but it cannot support both. With regular Ruby classes, it doesn't limit the usage and any future use cases for third party libraries.

Copy link
Contributor

@jensljungblad jensljungblad Jun 16, 2020

Choose a reason for hiding this comment

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

@bbugh Did you see my comment here? #348 (comment) I tried it out and I'm pretty sure it wont work as you want it to regardless :/ You cannot re-open a parent components slot specifically for a subcomponent. You need to redefine/replace the slot.

Copy link
Contributor

@rmacklin rmacklin Jun 19, 2020

Choose a reason for hiding this comment

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

What if the proc ends up being request-dependent? Allowing an arbitrary block of code opens us up to it executing whatever a developer wants, potentially making it difficult to precompile Slots.

I think we can/should prevent this. Not sure if this is what @jensljungblad had in mind, but I had been thinking we'd be calling the procs to resolve the associated classes within the compile method. That'd ensure they're only executed once, which means you can't do something request-dependent and would address the concern about them being slow.

Also I think the documentation could describe that the purpose of the proc is only to lazy-load the constant; if someone tries do so something crazy in the proc that would not be supported and they're on their own. (FWIW, they can already execute whatever they want by overriding const_get, which I think is also something we would not want to support.)

Would it really be that bad to just reference the Slot class name directly, without a proc? (like with_slot :header, Header) If we're loading a component, I think it's safe to assume that we want to load all of its slots.

I think that forces class Header to be defined before you call with_slot :header, Header, which was considered undesirable in #348 (comment). If we're okay with that restriction, that syntax would also work well regarding autocompletion, type-checking, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Like @rmacklin said, that was my only concern with with_slot :header, Header, the fact that the class has to be defined at the top of the file. It's not the end of the world.

I didn't look very closely at the compile method but yes, my hope was that the resolved class could be cached somehow.

Copy link
Contributor

Choose a reason for hiding this comment

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

My sense is it's powerful to have both options. I'm I'm the end user I'm likely to want to use the macros (I think of these as syntactic sugar/macros rather than a DSL). However, if I'm a Library writer I should use the API version and expose my slot classes IFF I want them to be extendable.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also have a pretty allergic reaction to using a Proc for the reasons @joelhawksley mentioned. I'm also concerned its not very "Rails" like I can't think of examples of this pattern. However, using strings for the names of classes is very common in AR and other places. It's not so convenient for autocompletion etc. but I'd take the safety and simplicity of the string over that.

Copy link
Contributor

@bbugh bbugh Jun 29, 2020

Choose a reason for hiding this comment

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

@jensljungblad the examples that you gave are not solutions to the problem I'm trying to explain. I will try to demonstrate at some point with real code.

lib/view_component/base.rb Outdated Show resolved Hide resolved
lib/view_component/base.rb Outdated Show resolved Hide resolved
@bbugh
Copy link
Contributor

bbugh commented Jun 16, 2020

@joelhawksley I haven't had a chance to try it yet, but after reviewing the docs and code, they look excellent! I think you did a great job encompassing everything we discussed. I'll spend some more time on it tonight.

joelhawksley and others added 2 commits June 16, 2020 12:58
Co-authored-by: Blake Williams <blakewilliams@github.com>
Copy link
Contributor

@jonspalmer jonspalmer left a comment

Choose a reason for hiding this comment

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

I like it a lot. Seems clean and compact.

Lots of places we could take this and I'm sure we will get there. I don't feel like we have enough concrete use cases just yet to really inform those choices. Until we do lets ship it and get folks using it in anger.

README.md Outdated Show resolved Hide resolved
lib/view_component/base.rb Outdated Show resolved Hide resolved
lib/view_component/base.rb Outdated Show resolved Hide resolved
slot_name
end

instance_variable_name = "@#{accessor_name}"
Copy link
Contributor

Choose a reason for hiding this comment

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

if we had a SlotDescription class as suggested elsewhere we could push some of this logic into that class.

with_slot :header, class_name: "Header"
with_slot :row, collection: true, class_name: "Row"

class Header < ViewComponent::Slot
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't help scratch the itch for mr that slots could themselves be ViewComponents. I've mentioned this before and I don't yet have the concrete uses cases (though i have my suspicions) so I'm prepared to let this go for a future release.

But here is my thinking. If you stripped out some of the magic around class_name and generating classes we're providing in with_slot you could imagine that you had to use slots like this:

<% component.slot(:header, HeaderSlot.new(class_names: "my-class-name")) do %>
    This is my header!
  <% end %>

the requirement on HeaderSlot being that it provide a content property to put the captured block in.

Under that light the component.slot is looking an awful lot like a render call on a ViewComponent.

Now imagine the guts of the slot method was something like this:

    slot = slots[slot_name]

    if args.is_a?(Hash)
      # The class name of the Slot, such as Header
      slot_class = self.class.const_get(slot[:class_name])

      # Instantiate Slot class, accommodating Slots that don't accept arguments
      slot_instance = args.present? ? slot_class.new(args) : slot_class.new
    else
      # args were really a class
      slot_instance = args
    end

    if slot_instance.responds_to(:render_in)
      # RenderInSlotWrapper exposes a `content` property that is the result of `render_in`
      # I'd need to think it through more but I think we need some sort of wrapper that 'captures' the entirety of the rendered ViewComponent
      slot_instance = RenderInSlotWrapper.new(slot_instance).render_in(view_context, &block) 
    else
      # Capture block and assign to slot_instance#content
      slot_instance.content = view_context.capture(&block) if block_given?
    end

My spidey senses says there is something power here.

# collection: true|false,
# class_name: "Header" # class name string, used to instantiate Slot
# )
def with_slot(*slot_names, collection: false, class_name: nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

My sense is it's powerful to have both options. I'm I'm the end user I'm likely to want to use the macros (I think of these as syntactic sugar/macros rather than a DSL). However, if I'm a Library writer I should use the API version and expose my slot classes IFF I want them to be extendable.

@thomasbrus
Copy link

@joelhawksley @jonspalmer Probably a bit late to chime in but I think all of this can be accomplished without introducing a new DSL.

class BoxComponent < ViewComponent::Base
  def header(*args, &block)
    # Could also use content_tag with a .box__header class name here.
    # Point is that args / options are passed along
    helpers.render(BoxHeaderComponent.new(*args), &block)
  end

  def row(*args, &block)
    # ... renders .box__row
  end

  def footer(*args, &block)
    # ... renders .box__footer
  end
end
= render(BoxComponent.new) do |box|
  = box.header(class: 'bg-blue text-white') do
    | Title

  = box.row do
    | Row 1

  = box.row do
    | Row 2

  = box.footer do
    | Footer

Yes, it doesn't enforce the order of header / row / footer, like a content_area does. But I think that is fine. The flexibility comes in quite handy at times actually.

@jensljungblad
Copy link
Contributor

@joelhawksley @jonspalmer Probably a bit late to chime in but I think all of this can be accomplished without introducing a new DSL.

Yes, it doesn't enforce the order of header / row / footer, like a content_area does. But I think that is fine. The flexibility comes in quite handy at times actually.

But you can still do this with or without slots, right? This should be possible with the current content block?

@thomasbrus
Copy link

thomasbrus commented Jun 28, 2020

But you can still do this with or without slots, right? This should be possible with the current content block?

Yes exactly! That's the point, it doesn't need a new API like slots. In this case the template would be:

.box
  = content

Another example is a Tabs component, it uses the pattern above, plus content areas:

# app/components/tabs_component.rb
class TabsComponent < ApplicationComponent
  with_content_areas :navigation, :panels

  def item(*args, &block)
    # Renders a single tab (item)
    helpers.render(TabsItemComponent.new(*args), &block)
  end

  def panel(*args, &block)
    # Renders tab panel
    helpers.render(TabsPanelComponent.new(*args), &block)
  end
end
/ app/components/tabs_component.html.haml
.tabs(data-controller="tabs")
  .tabs__navigation
    = navigation

  .tabs__panels
    = panels
/ example usage
= render(TabsComponent.new) do |tabs|
  - tabs.with(:navigation) do
    = tabs.item(id: :tab1, active: true) do
      Tab 1
    = tabs.item(id: :tab2) do
      Tab 2
    = tabs.item(id: :tab3) do
      Tab 3

  - tabs.with(:panels) do
    = tabs.panel(id: :tab1, active: true) do
      First panel
    = tabs.panel(id: :tab2) do
      Second panel
    = tabs.panel(id: :tab3) do
      Third panel

I'm not saying Slots is a bad idea per se, it is just that the issue stated in the Problem Statement can already be solved quite nicely with what have now.

@bbugh
Copy link
Contributor

bbugh commented Jun 28, 2020

Sorry I'm way behind on all of the discussions here y'all, I've been tied up by some life stuff. I'll try to get some time to catch back up this weekend.

Hi Thomas! 👋 We have discussed that previously! Don't worry though, this is an insidiously more complex problem than it first appears and I'd guess that everyone who has participated in these discussions has gone through a long process of repeating "can't you just..." through all of the obvious-but-flawed solutions until we've ended up here together.

In your example, the issue I see that this proposal solves is that we would still have the need to make a lot of tiny tightly-coupled-but-loosely-grouped subcomponent files, which could be avoided with slots. I outlined scenarios for why slots are better here, take a look and see if that helps shine some light on how we've come to the present proposal: #325 (reply in thread)

On the general topic of "why slots" -

In React, the "micro component" problem is easily solvable with a one-liner functional component: export const Panel = (id) => <div>... React embeds their templates in the code, so it's easy to group all of this together in the same file, and making a new component is super easy and cheap, since it's just another line of JavaScript. In view_component, this is not the case and it gets soupy quickly. We have .rb, .html.erb/haml/slim, then if you include sidecar files you also get .js, and .css, and the current design suggests putting all components in one big folder. Requiring that every little sub-section also be its own component files will quickly become an unpleasant mess.

In Vue (which Rails/ERB more closely resembles than React), the need for small bits of embedded templating is instead solved by using slots. You end up with the same kind of benefits, in that you can keep all of your logic and templates grouped together in the same place without spreading it across lots of files, but it's a very different solution and (imo) has more advantages. While slots resemble a "mini component", in practice they are quite different. They can be optional, you can have defaults, they easily pass data up the tree (important!), the "owner" component can easily refactor the internals without requiring consumers to change their code, they're lightweight, components are more self-contained which (imo) makes long-term maintenance easier, etc.

Ruby's way of building components much more closely resembles Vue, and I think taking inspiration from it over React to solve these issues will result in a better library. Ruby also has the advantage that its ERB templates has the full expressive power of the Ruby language without needing a DSL like Vue's directive/handlebars one, and the syntax for slots is built into the language.

@thomasbrus
Copy link

@bbugh Hey, thanks for your lengthy reply!

this proposal solves is that we would still have the need to make a lot of tiny tightly-coupled-but-loosely-grouped subcomponent files,

Not necessarily, it can just be a call to content_tag really (see comment in #header method) :)

class BoxComponent < ViewComponent::Base
  def row(**opts, &block)
    helpers.content_tag(:div, deep_merge_html_options(opts.fetch(:html, {}), class: 'box__row'), &block)
  end
end

In general I agree about that it is not desirable to have tons of tiny subcomponents. I think in the case of a TabsItemComponent and TabsPanelComponent it is justified because these are actually somewhat complex in how they behave and are styled. They can be tested separately. (Btw, in our codebase we happen to have to convention to not define classes inline.)

I agree about that in React it is much easier to locally scope styling and subcomponents, and other assets. Which is great. The "rails" way is to just have a folder for each type.. app/controllers, app/components, app/assets etc. But for a view layer that is component-based, within a Rails application, a nested approach might be desirable yes.

I understand the idea of looking at other frameworks/libraries to port some of the "proven" ideas that worked for them and to see if we can apply them here. It is actually a great approach 🚀

Purely looking at the Problem Statement at the start of the issue, I think a plain Ruby approach suffices ("my" proposal, for lack of a better name). Not to say Slots don't provide additional benefits still.

@joelhawksley
Copy link
Member Author

@bbugh @jensljungblad @jonspalmer @BlakeWilliams @thomasbrus thank you all for your feedback and discussion!

I've made the changes I think we should make ahead of landing this PR, including moving Slots to ViewComponent::Slotable so that folks must opt-in to using Slots for now.

I think we've exhausted the benefits of iterating on this PR and should focus our energy on testing what we have here in our applications. Could you provide a final review before I merge?

Copy link
Contributor

@nickh nickh left a comment

Choose a reason for hiding this comment

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

Thanks to everyone for the discussion and review, I'm excited to start using this!

Copy link
Contributor

@BlakeWilliams BlakeWilliams left a comment

Choose a reason for hiding this comment

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

LGTM!

lib/view_component/slotable.rb Outdated Show resolved Hide resolved
Co-authored-by: Blake Williams <blakewilliams@github.com>
Copy link
Contributor

@jensljungblad jensljungblad left a comment

Choose a reason for hiding this comment

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

Wow, that was some discussion! Thanks @joelhawksley for facilitating and getting it done.

Copy link
Contributor

@bbugh bbugh left a comment

Choose a reason for hiding this comment

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

Three "maybe needed" changes.

Comment on lines +171 to +173
Normal slots can be rendered once per component. They expose an accessor with the name of the slot that returns an instance of `BoxComponent::Header`, etc.

Collection slots can be rendered multiple times. They expose an accessor with the pluralized name of the slot (`#rows`), which is an Array of `BoxComponent::Row` instances.
Copy link
Contributor

Choose a reason for hiding this comment

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

Named slot subclasses aren't automatically generated after cc4ddc8 right?

Suggested change
Normal slots can be rendered once per component. They expose an accessor with the name of the slot that returns an instance of `BoxComponent::Header`, etc.
Collection slots can be rendered multiple times. They expose an accessor with the pluralized name of the slot (`#rows`), which is an Array of `BoxComponent::Row` instances.
Normal slots can be rendered once per component. They expose an accessor with the name of the slot that returns an instance of `ViewComponent::Slot`, etc.
Collection slots can be rendered multiple times. They expose an accessor with the pluralized name of the slot (`#rows`), which is an Array of `ViewComponent::Slot` instances.

Comment on lines +21 to +23
# Hash of registered Slots
class_attribute :slots
self.slots = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like this might belong in the new Slotable module instead of base?

@@ -181,6 +186,10 @@ def inherited(child)
# has been re-defined by the consuming application, likely in ApplicationComponent.
child.source_location = caller_locations(1, 10).reject { |l| l.label == "inherited" }[0].absolute_path

# Clone slot configuration into child class
# see #test_slots_pollution
child.slots = self.slots.clone
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like it might also belong in the Slotable module?

Copy link
Member Author

Choose a reason for hiding this comment

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

In theory, I agree. I avoided doing so as I didn't want to re-define included in the module.

@bbugh
Copy link
Contributor

bbugh commented Jun 30, 2020

I've been hearing the Shots song in my head while reviewing this. SLOTS SLOTS SLOTS SLOTS SLOTS! SLOTS SLOTS SLOTS SLOTS SLOTS!

This has really come together, great work! 🎉 I agree that any further discussion on this PR will have diminishing returns, and a chance to do some concrete usage will be very beneficial to further exploration. Can't wait to try it out!

@joelhawksley joelhawksley merged commit b05c775 into master Jun 30, 2020
@joelhawksley joelhawksley deleted the slots branch June 30, 2020 17:13
@joelhawksley
Copy link
Member Author

@bbugh for some reason I didn't see your review until now! Will address in a quick follow-up PR.

Copy link
Contributor

@jonspalmer jonspalmer left a comment

Choose a reason for hiding this comment

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

I'm late to the party but for what it's worth this looks great

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.

8 participants