-
Notifications
You must be signed in to change notification settings - Fork 444
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
Conversation
1479927
to
b418b1d
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.
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!
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 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 Also, I wonder if maybe the subclass should have a domain suffix like Thanks for putting this together @joelhawksley. I'm excited to get some time to test it out soon! |
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.
The slot
/slot_names
bug is a blocker.
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 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:
- Always require using
with_slots
(should this bewith_slot
btw? Similar toattr_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.
- 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.
Hey @jensljungblad, really appreciate you chiming in on this since you already have domain knowledge about the tribulations of designing a Rails component library!
Off the top of my head, the only place I know for sure in Rails that generates dynamic classes is 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 |
Another potential benefit with passing the class to 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. # 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 |
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:
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
<li class="Box-row <%= row.theme_class_name %>">
<%= row.content %>
</li>
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
<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 |
Another thought on the API. Do we need |
So the requirement is to allow anonymous and user defined slot classes. I believe these are the suggestions: 1. Auto-register classesclass 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:
Drawbacks:
2. Use macro to point to user defined slot classInfer 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:
Drawbacks:
3. Use macro to point to user defined slot class but without inferring nameNo 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:
Drawbacks:
3.1 Use macro to point to user defined slot class but without inferring name, with collection argumentVariation on 3 with multiple arguments to 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:
|
@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 (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) |
I experimented a little myself in #354. I've got one general question:
As for my experiment, two things that would need addressing:
|
@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.
I'm a big fan of the def self.with_slot(*slot_names, class: ViewComponent::Slot, collection: false) In addition I'd add the wrinkle that # 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 |
👍
It actually relates to your suggestions. Consider: class Box < Component
with_slot Header
class Header; end
end This wont work, since the 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 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 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 |
I like the API where each line defines a slot(s) and can take options. It looks like 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.
I think this is a realistic evolution based on some of what we're discussing here. How would you see this progressing?
We discussed this in the pull request review here, the gist is that I (and @jonspalmer at the time) agree that
I believe the reasons raised against this idea before was that ViewComponents require templates and can be created by
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. |
Here's an example of resolving the slot classes in the way I mentioned above: 09779e1 |
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 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. |
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.
@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 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 |
I agree 100%, if we are going to take the API in the direction that is being suggested here. I agree that
Agreed. |
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
Hm, good point. I guess the requirements are:
And some options:
with_slot :header, class_name: "Header"
class Header; end
with_slot :header
class Header; end
with_slot :header
class HeaderSlot; end
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 |
Here's a commit demonstrating option 4) in the previous comment: 614a4b8. |
I was going to say, what if the class is namespaced, as in 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:
|
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.
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 |
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 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.
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 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.
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 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.
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.
@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 %>
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 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 %>
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.
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
# collection: true|false, | ||
# class_name: "Header" # class name string, used to instantiate Slot | ||
# ) | ||
def with_slot(*slot_names, collection: false, class_name: nil) |
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.
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.
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.
@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?
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.
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".
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 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.
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.
@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.
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 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.
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.
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.
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.
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.
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 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.
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.
@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.
@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. |
Co-authored-by: Blake Williams <blakewilliams@github.com>
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 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.
lib/view_component/base.rb
Outdated
slot_name | ||
end | ||
|
||
instance_variable_name = "@#{accessor_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.
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 |
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 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.
lib/view_component/base.rb
Outdated
# collection: true|false, | ||
# class_name: "Header" # class name string, used to instantiate Slot | ||
# ) | ||
def with_slot(*slot_names, collection: false, class_name: nil) |
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.
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.
@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 |
But you can still do this with or without slots, right? This should be possible with the current |
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. |
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: 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. |
@bbugh Hey, thanks for your lengthy reply!
Not necessarily, it can just be a call to content_tag really (see comment in 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 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.. 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. |
Co-authored-by: Jon Palmer <328224+jonspalmer@users.noreply.github.com>
@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 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? |
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.
Thanks to everyone for the discussion and review, I'm excited to start using this!
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.
LGTM!
Co-authored-by: Blake Williams <blakewilliams@github.com>
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.
Wow, that was some discussion! Thanks @joelhawksley for facilitating and getting it done.
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.
Three "maybe needed" changes.
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. |
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.
Named slot subclasses aren't automatically generated after cc4ddc8 right?
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. |
# Hash of registered Slots | ||
class_attribute :slots | ||
self.slots = {} |
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.
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 |
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.
This seems like it might also belong in the Slotable
module?
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.
In theory, I agree. I avoided doing so as I didn't want to re-define included
in the module.
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! |
@bbugh for some reason I didn't see your review until now! Will address in a quick follow-up PR. |
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 late to the party but for what it's worth this looks great
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:
With Content Areas, this could be implemented as:
# box_component.rb
# box_component.html.erb
There are some shortcomings to this approach:
Collections
Having to pass to pass in
rows
toBoxComponent
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
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
# box_component.html.erb
# index.html.erb
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
, andfooter
) that returns an instance ofBoxComponent::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 ofBoxComponent::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