-
Notifications
You must be signed in to change notification settings - Fork 28
Feature: Extend WrapperComponent to support it being conditional on multiple components
#57
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
base: master
Are you sure you want to change the base?
Conversation
|
Hey @osjjames, Thanks for the proposal. I like the idea and see how it can be helpful (thanks for the detailed examples). However, the Also, I think, we can still re-use the existing Here is what I have in mind: <%= render ViewComponentContrib::WrapperComponent.new do |wrapper| %>
<div class="flex flex-col gap-4">
<h3>Examples</h3>
<div class="flex gap-2">
<%= render ExampleA::Component.new.wrapped_in(wrapper) %>
<%= render ExampleB::Component.new.wrapped_in(wrapper) %>
</div>
</div>
<%- end -%>First, we don't pass any components to the Then, we introduced the One important difference here (and, I think, this is crucial) is that we don't rely on the HTML content but on the WDYT? |
|
Hi @palkan, thanks for the review! Yeah, the verbosity was something that I wasn't happy with either. Your suggested API is a big improvement! I'll update this PR to reflect that. Should be done within a few days - I'll give you a shout if I'm stuck on anything 👍 |
|
Hi @palkan, ready for you to take another look 🙌 I've adopted the API you described, as well as adding a |
ShowIfWrapperComponent to support wrappers conditional on multiple componentsWrapperComponent to support it being conditional on multiple components
|
Looks great! I'd only suggest renaming |
|
Done 👌 happy to do further docs/changelog updates if needed |
|
@palkan bump - anything else needed? |
What is the purpose of this pull request?
Fixes #56.
What changes did you make? (overview)
#wrapped_inmethodExtends the
ViewComponentContrib::WrapperComponentto support many dependent child components, such that if none of the registered child components return true from theirrender?method, the wrapper component will also not render.This is achieved through a helper method for components called
#wrapped_in.For example, consider here that we only want the content to be rendered if either
ExampleAorExampleBis rendered.It also supports deep nesting of conditional wrappers.
Imagine we have a large section called "Examples", with two sub-sections: "Foo Examples" and "Bar Examples". Each sub-section should only render if it has at least one component rendered inside it, and the large section should only render if at least one sub-section is rendered. We can achieve the behaviour like this:
#placeholdermethodAdds a public method to the
ViewComponentContrib::WrapperComponentthat allows some placeholder content to be rendered only if none of the wrapper's registered components render.This furthers the goal of keeping conditionals out of the template, and utilises the recursive evaluation of
render?methods.How it works
When
#wrapped_inis called on a component, the wrapper component stores it in an array calledregistered_components. The wrapper's#render?method then simply callsregistered_components.any?(&:render?)and uses that result.A consequence of this is that we lose the benefit of ViewComponent's lazy render evaluation. Child components need to be evaluated in order for
#wrapped_into be called, so aWrapperComponentmust have all of its contents evaluated before render.Is there anything you'd like reviewers to focus on?
WrapperComponent? At the moment it has two "modes" that are mutually exclusiveChecklist