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

Content area options #4

Closed
domchristie opened this issue Dec 7, 2020 · 11 comments · Fixed by #57
Closed

Content area options #4

domchristie opened this issue Dec 7, 2020 · 11 comments · Fixed by #57

Comments

@domchristie
Copy link
Contributor

domchristie commented Dec 7, 2020

When rendering component-like partials, it'd be nice to customise element attributes within the partial, not just the content. For example, let's say we have a section partial, with a <header> element, and we can set its content via p.content_for :header. What if we want to customise the <header> element itself in a particular situation? A Rails-y API might look like

<%= render 'section' do |p| %>
  <% p.content_for :header, class: 'sm:hidden' %>
  <p>Lorem ipsum…</p>
<% end %>

With this API, we'd need to have some kind of name-spaced store for the options passed into content_for, and a method to access them. Perhaps something like p.options_for(:name):

<%# app/views/application/_section.html.erb %>
<% content = yield(p = np) %>
<section>
  <% if p.content_for?(:header) %>
    <%= content_tag :header, p.content_for(:header), p.options_for(:header) %>
  <% end %>
  
  <%= content %>
</section>

I think this could be accomplished with something like:

class Partial
  def initialize(view_context)
    @view_context = view_context
    @key = SecureRandom.uuid
    @options = {}
  end

  def content_for(name, content_or_options_with_block = nil, options = {}, &block)
    if block_given?
      content = capture(&block)
      options = content_or_options_with_block || {}
    else
      content = content_or_options_with_block
    end

    @options[name_for(name)] = options.without(:flush)
    @view_context.content_for(name_for(name), content, options)
  end
  
  def options_for(name)
    @options[name_for(name)]
  end
  
  def name_for(name)
    "#{name}_#{@key}".to_sym
  end
end

This expands the API beyond the current mirrored approach, but thought I'd throw it out there and see what you make of it.

@domchristie
Copy link
Contributor Author

domchristie commented Dec 8, 2020

Somewhat related: looks like combining/merging DOM token lists (and therefore some content_tag options) in Rails is going to get a little easier in Rails 6.1: rails/rails#40146 and in particular: rails/rails#40146 (comment)

@andrewculver
Copy link
Contributor

@domchristie I don't think we necessarily need a new option to accomplish the outcome desired above. For example, my first instinct would be to try and accomplish the same thing just using locals, like so:

<% render 'section', header: {class: 'sm:hidden'} do |p| %>
  <% p.content_for :header do %>
    Some header content.
  <% end %>

  <% p.content_for :body do %>
    <p>Lorem ipsum…</p>
  <% end %>
<% end %>

And the partial would be something like:

<% yield p = np %>

<% header ||= {} %>

<section>
  <% if p.content_for?(:header) %>
    <header class="some-default-class another-default-class <%= header[:class] %>">
      <%= p.content_for(:header) %>
    </header>
  <% end %>
  
  <%= p.content_for(:body) %>
</section>

In practice this has actually worked pretty well for me. In particular, you have a lot of control within the partial itself what you want to do with the locals that are passed in. You can, for example, merge them into a default version of a hash that's defined in the partial. You can overwrite the default values. You can append to the default values (which works well for utility classes, for example). I've got a bunch of examples of different things that can be accomplished from the Bullet Train code base and other projects I've worked on. I'll try to find a moment to put them into the documentation.

Happy to continue discussing, but generally my instinct here is for folks to accomplish as much as possible using the native approach they would have taken with partials in the first place.

@andrewculver
Copy link
Contributor

(Please let me know if there is an angle I'm missing here.)

@domchristie
Copy link
Contributor Author

I'm a bit torn. On the one hand, using locals requires no extra code, and it keeps the library code nice and clean; on the other, we're polluting locals with attributes that don't directly apply to the containing "component"—rather, they apply to the child content areas. There is also potential for a partial with lots of content areas to have an unwieldy list of locals. Allowing p.content_for to accept applicable options keeps the main locals options tidy whilst keeping the content area options close to the elements they affect e.g.:

<%= render 'section', data: { component: 'chat', props: { id: 1 } }, toolbar: { class: 'toolbar--chat', data: { component: 'toolbar', props: { actions: […] }.to_json } } do |p| %>
  <%#%>
  <%#%>
  <%#%>
  <%#%>
  <%#%>
  <%#%>
  <% p.content_for :toolbar do %>
    <%#%>
  <% end %>
<% end %>

versus:

<%= render 'section', data: { component: 'chat', props: { id: 1 } } do |p| %>
  <%#%>
  <%#%>
  <%#%>
  <%#%>
  <%#%>
  <%#%>
  <% p.content_for :toolbar, class: 'toolbar--chat', data: { component: 'toolbar', props: { actions: […] }.to_json } do %>
    <%#%>
  <% end %>
<% end %>

Inside the partial, we'll have just as much control over the options since it's just whatever was passed in to p.content_for.

Perhaps there's a better API for this that doesn't move away from the elegance of just wrapping content_for? I'll have a think.

@andrewculver
Copy link
Contributor

Yeah, I see your point. I'll keep thinking on this one, too.

@domchristie
Copy link
Contributor Author

I should add, I'm looking into migrating from a hand-rolled component framework to Nice Partials, and stress-testing it on a layout component. The very shortened code I'm testing looks like:

<%= section data: { component: '…' } do |s| %>
  <%= s.toolbar class: '…' do %>
    <%#%>
  <% end %>
<% end %>

Which would ideally convert to something like:

<%= render 'section', data: { component: '…' } do |p| %>
  <%= p.content_for :toolbar, class: '…' do %>
    <%#%>
  <% end %>
<% end %>

@andrewculver
Copy link
Contributor

@domchristie Not sure how I didn't grok this the first time I looked at it, but this is actually fantastic. I totally get this now. It's just local locals, but for the named content areas/slots. Do you have a PR by chance?

@domchristie
Copy link
Contributor Author

No PR atm, but I've had a quick play with callbacks. It still feels Rails-y. This is what it could look like:

class ApplicationPartial < NicePartials::Partial
  before_capture :set_content_options

  def options_for(name)
    content_options[name_for(name)] || {}
  end

  private

  def content_options
    @content_options ||= {}
  end

  def set_content_options
    content_options[name_for(current_name)] = current_options
  end
end

@domchristie
Copy link
Contributor Author

Obviously it would require overriding np to return an ApplicationPartial instance, but I wonder if that might help with people understanding the magic? An ApplicationPartial may also be useful for developers to add methods that's useful to all their Nice Partials, saving them from repetition in p.helpers do …

@andrewculver
Copy link
Contributor

@domchristie Hey, sorry this took me so long to circle back around on, but I'm convinced you were right about this. For example, we might have a content area on a component that defaults to having padding, but we want to allow downstream component users to specify that the default padding should be disabled, e.g.:

  <% p.content_for :footer, padding: false do %>
    ...
  <% end %>

@domchristie
Copy link
Contributor Author

No worries! Overriding component styles it something I've been mulling over (and I think is related to: #13).
Also, did you see this tailwind proposal: https://twitter.com/adamwathan/status/1496663833980686338?

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