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

Custom content sections #27

Closed
wants to merge 5 commits into from
Closed

Custom content sections #27

wants to merge 5 commits into from

Conversation

kaspth
Copy link
Contributor

@kaspth kaspth commented Jun 20, 2022

This adds a layer on top NicePartial::Partials content_for API to have custom named sections and storing more than just string content. A slightly different take to #9

Here we define a title content section with some options that we can then refer to later and use to build other content:

<%= yield p = np %>
<% p.title class: "some-class", data: { controller: "yup" } %>
<%= tag.span **p.title.options %>

With this abstraction we could potentially also allow merging two NicePartial::Partials to allow for #11, so delegation really becomes copy the passed in sections to the new NicePartial::Partial or something like that.

Also curious what @domchristie and @seanpdoyle thinks of something like this, since they've done a lot of work in this area.

@domchristie
Copy link
Contributor

Just having a play with this now. Is there a new API for setting content? I have a card partial (as above) and a template that renders it:

<%= render "card" do |card| %>
  <% card.content_for :title, "Hello, world!" %>
<% end %>

but this just renders:

<span class="some-class" data-controller="yup"></span>

p.title.inspect:

 #<NicePartials::Partial::Section:0x00000001239f6c20 @content="", @options={:class=>"some-class", :data=>{:controller=>"yup"}}> 

@andrewculver andrewculver force-pushed the clarify-translation-context-popping branch from fae9624 to 35448a1 Compare June 27, 2022 15:58
@kaspth kaspth force-pushed the clarify-translation-context-popping branch from 35448a1 to 9fa5892 Compare June 28, 2022 12:43
end

def yield(name = nil)
raise "You can only use Nice Partial's yield method to retrieve the content of named content area blocks. If you're not trying to fetch the content of a named content area block, you don't need Nice Partials! You can just call the vanilla Rails `yield`." unless name
content_for(name)
public_send(name)
Copy link
Collaborator

@seanpdoyle seanpdoyle Jun 29, 2022

Choose a reason for hiding this comment

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

Is this style of interface compatible with yield/content_for/partial.named_block pairings that yield arguments?

On top of capturing options for a content_for block, it might be useful to yield instances of pre-configured builders.

For example, an ActionView::Helpers::FormBuilder instance built by form_with or fields_for, or an instance of tag that's pre-configured with some attributes:

<% yield p = np %>

<dialog aria-labelledby="dialog_title">
  <header>
    <%= p.yield(:title, tag.with_options(id: "dialog_title", class: "text-lg")) %>
  </header>
   
  <%= tag.div(p.content, p.content.options) %>

  <footer>
    <% tag.with_options(class: "rounded-full p-4") do |styled_tag| %>
      <form method="dialog"><%= styled_tag.button("Cancel") %></form>
      <%= p.yield(:actions, styled_tag) %>
    <% end %>
  </footer>
</dialog>

<!-- Elsewhere -->
<%= render "dialog" do |dialog| %>
  <%= dialog.title do |title| %>
    <%= title.h1("A title") %>
    <%# => <h1 id="dialog_title" class="text-lg">A title</h1> %>
  <% end %>

  <%= dialog.content(class: "text-md") do %>
    Content with options but not block arguments
  <% end %>
  <%# => <div class="text-md">Content with options but not block arguments</div> %>

  <%= dialog.actions do |action| %>
    <form method="post" action="/messages">
      <%= action.button("Send") %>
      <%# => <button class="rounded-full p-4">Send</button> %>
    </form>
  <% end %>
<% end %>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this style of interface compatible with yield/content_for/partial.named_block pairings that yield arguments?

I added the yield backing for backwardscompatiblity, but it's possible it's clearer to separate that storage. I wouldn't consider this to be encouraged p.yield(:title, tag.with_options(id: "dialog_title", class: "text-lg")), personally I find that really confusing because it doesn't yield. Is Rails' yield :title, tag.with_options(…) equivalent to content_for :title, tag.with_options(…)? I wasn't aware it supported that!

On top of capturing options for a content_for block, it might be useful to yield instances of pre-configured builders.

Oh this is interesting! It's taking me a bit to grok your example — once we get into yielding and nesting it gets heady for me — but I think I'm getting it now. So you could pass arguments on that then become available as block parameters, that's really nice. I'll take a swing at that.

For example, an ActionView::Helpers::FormBuilder instance built by form_with or fields_for

How are you seeing it in this case?

Copy link
Collaborator

@seanpdoyle seanpdoyle Jun 29, 2022

Choose a reason for hiding this comment

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

Is Rails' yield :title, tag.with_options(…) equivalent to content_for :title, tag.with_options(…)? I wasn't aware it supported that!

I believe that's the case. To quote the content_for documentation:

You can then use content_for :not_authorized anywhere in your templates.

<%= content_for :not_authorized if current_user.nil? %>

This is equivalent to:

<%= yield :not_authorized if current_user.nil? %>

content_for, however, can also be used in helper modules.

I chose p.yield in my example to mimic the yield style of content_for. It's ironic, but I get confused by the difference in behavior between content_for with a single argument and content_for with multiple arguments. I find yield to more clearly communicate the "capture" intent, in spite of its overloading or Ruby's yield keyword.

With that being said, I think the pseudo-code example would serve the same purpose if calls to p.yield were replaced with p.content_for. I think calls to p.named_block could get messy or confusing, since it'd no longer be straightforward to discern between a p.named_block that intends to capture content by providing a block argument, and a p.named_block that provides content:

<% yield p = np %>

<!-- declared in the partial, intends to yield an object to the caller -->
<header><%= p.title tag.with_options(class: "text-lg") %></header>

<!-- declared at a call site, intends to provide test to the partial -->
<%= p.title "My title text" %>

So you could pass arguments on that then become available as block parameters, that's really nice.

There's a somewhat obscure example of yielding an object to a block in the _layout_for documentation.

For example, an ActionView::Helpers::FormBuilder instance built by form_with or fields_for

How are you seeing it in this case?

I'm imagining something like an application-wide form partial:

<%# app/views/application/_form.html.erb %>

<% form_options = local_assigns.slice(:model, :url, :id, :data, :html) %>

<% yield p = np %>

<%= form_with some_magic_helper_that_can_deeply_merge_attributes_and_tokens(form_options, data: { controller: "form", action: "submit->form#doSomethingSpecial" }) do |form| %>
  <%= p.yield :controls, form %>

  <div>
    <%= link_to "Cancel", :back %>
    <%= p.yield :actions, form %>
  </div>
<% end %>

<%# elsewhere %>

<%= render "form", model: @post, data: { controller: "special-form", action: "reset->special-form#resetInASpecialWay" } do |builder| %>
  <%= builder.controls do |form| %>
    <%= form.label :name %>
    <%= form.text_field :name %>
    <%# ... %>
  <% end %>

  <%= builder.actions do |form| %>
    <%= form.button "Reset", type: "reset" %>
    <%= form.button "Submit" %>
  <% end %>
<% end %>

@kaspth kaspth force-pushed the clarify-translation-context-popping branch from 58504e9 to 38ba582 Compare June 29, 2022 14:03
Base automatically changed from clarify-translation-context-popping to main June 29, 2022 14:21
@kaspth
Copy link
Contributor Author

kaspth commented Jun 29, 2022

@domchristie yeah, the card template I showed doesn't render an element, it just render some options because I wanted to showcase the difference. <%= tag.h1(p.title, p.title.options) %> would be how to render the element (but there's also no title content defined in that template).

@kaspth kaspth closed this Jun 29, 2022
@kaspth kaspth reopened this Jun 29, 2022
Rails' content_for delegates its storage to @view_flow,
an instance of ActiveSupport::OutputFlow. It's really just a
wrapper around a hash of ActiveSupport::SafeBuffers.

https://github.com/rails/rails/blob/b8bd24d93825684e2311d085266cf66b4fb45368/actionview/lib/action_view/flows.rb#L10

I think we're better off inlining the indirection here and take on some
of content_for's implementation. It'll help setup some extra ideas I'll
be trying next.
@kaspth kaspth marked this pull request as draft June 29, 2022 17:14
@domchristie
Copy link
Contributor

@kaspth ah got it! Sorry I was getting confused with whether the partial was setting, or rendering :/ (and calling <% p.title class: … %> overrides any previous call, so content becomes blank)

In terms of the API, there is a little history to this. Here's my ramblings from a while back:

ViewComponent has gone with component.slot_name (rather than component.slot(:slot_name)

I suppose we could do something similar…

set content with: <% p.content_for :slot_name %> and then render with <%= p.slot_name %>

Echoing #4 (comment)

In terms of semantics, is this really correct? e.g.
<%= content_tag(:h2, p.header) if p.header? %>

the header is the element, wheres p.header holds the content for the header

I suppose it depends on how much work the partial does in terms of mark up


I've never been that keen on the yield method because of potential conflicts with Ruby's yield, but saying that, I too get confused with whether I'm setting or getting. On the other hand, I like that nice_partials feels Rails-y and familiar by using content_for/yield 🤷‍♂️

(Apologies if this isn't particularly helpful!)

@domchristie
Copy link
Contributor

domchristie commented Jun 30, 2022

I think this also changes the content_for behaviour to be more like provide. Might be unusual to do this…

Before

<%= render "card" do |card| %>
  <% card.content_for :title, "Hello" %>
  <% card.content_for :title, "world" %>
<% end %>

<%# in partial %>
<%= card.content_for :title # Helloworld %>

After

<%= render "card" do |card| %>
  <% card.content_for :title, "Hello" %>
  <% card.content_for :title, "world" %>
<% end %>

<%# in partial %>
<%= card.content_for :title # world %>

@kaspth
Copy link
Contributor Author

kaspth commented Jul 3, 2022

@domchristie sure, good point. I was going off the README that used the flush: true examples and figured we didn't want to support appending to a section. Though I've extracted the first commit into #42 and fixed this so we're appending like content_for does.

@kaspth
Copy link
Contributor Author

kaspth commented Jul 4, 2022

Closed in favor of #43 though there may be more parts we want to grab from here.

@kaspth kaspth closed this Jul 4, 2022
@kaspth kaspth deleted the custom-content-sections branch July 4, 2022 13:18
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.

None yet

3 participants