ViewComponent / demo

Rails application with ViewComponent
MIT License
157 stars 15 forks source link

Make the integration a gem #1

Closed jcoyne closed 5 years ago

jcoyne commented 5 years ago

I'd love to see this packaged as a gem so that it can be reused by the broader community. I wonder if you could ask Godfrey Chan if he would be willing to relinquish actionview-component

joelhawksley commented 5 years ago

@jcoyne I totally agree!

I was able to ask @chancancode in person at RailsConf about using the actionview-component name, and he said he'd be willing to hand it over.

That being said, I'm not sure what form this could/should take as a gem. I'm cautious about including writing one with the monkey patch as it stands. Perhaps there would be a way to ship it without the monkey patch? What do you think?

That all being said, we plan to continue our work towards upstreaming this, so the need for it in gem form would in theory be temporary.

cc @mohaballi

jcoyne commented 5 years ago

@joelhawksley I think its reasonable to ship a gem that has a monkeypatch, but maybe you also want to open an issue/PR in rails to make it possible to configure the render without a monkeypatch.

dylanahsmith commented 5 years ago

An easy way to avoid the monkey patch would be to just use a different method name. E.g. define a render_component view helper method instead of overloading render. That also allows you to remove the conditional logic in the method itself (i.e. you could just remove return super unless component.is_a?(Class) && component < ActionView::Component).

Given the simplicity of what is left (component.html(self, *args, &block)), another option would be to just call a class method on the component class directly. The view_context argument isn't even needed if the component doesn't take a block, so avoiding the need to pass this argument would make their usage simpler (e.g. render Issues::Badge, state: issue.state.to_sym could become Issues::Badge.render(state: issue.state.to_sym)). A separate base class for block components could be used (e.g. ActionView::BlockComponent) which could take the view context as the first argument.

joelhawksley commented 5 years ago

@dylanahsmith that's a fair point. There are several avenues we could take here by changing the API to not need a monkey patch.

At this point, we're working through a few edge cases we've seen on our end, but have our sights on shipping a gem with the monkey patch.

rafaelfranca commented 5 years ago

I'd just upstream this at this point. I guess a gem would not hurt since this feature would only be released in 6.1, so it would be good to give early access to the feature.

I'm happy to streamline a patch upstream and we can work on those edge cases directly in Rails. As we are in the early stages of a new version, now it is the perfect time.

@joelhawksley do you want to open a PR with what you have to at least start the conversation?

joelhawksley commented 5 years ago

@rafaelfranca I'd love to go that route. I'll be in touch!

rmacklin commented 5 years ago

👋 I've been silently subscribed to this repository since seeing your talk. My team has done some pretty similar work and I'm eager to follow the progress of ActionView::Component (as we'd ultimately want to migrate to the implementation that ends up in rails) - will you keep us posted after you and Rafael sync up? Very excited about this!

joelhawksley commented 5 years ago

@rmacklin of course! Can you provide any feedback on what we have here based on the work your team has done?

rmacklin commented 5 years ago

Sorry for missing this! I'll share some details about things we've done later today

joelhawksley commented 5 years ago

@rmacklin I opened a PR on Rails this morning: https://github.com/rails/rails/pull/36388 😄

rmacklin commented 5 years ago

You did an awesome job on that PR! 👍 👍


So I looked through our stuff and while there’s a lot I could share, I think most of it is conceptually pretty similar to ActionView::Component. However, there is one thing that we also did which is a bit different, that might be of interest.

While I don’t mind the render Issues::Badge syntax, we also adopted a syntax backed by helper methods, which turns:

<%= render PullRequestBadge, state: issue.pull_request.state.to_sym, is_draft: issue.pull_request.draft? %>

into something like:

<%= pull_request_badge state: issue.pull_request.state.to_sym, is_draft: issue.pull_request.draft? %>

One thing we liked about this syntax is that it looks very much like a custom element (if you squint, you might not see a difference 😉). To that end, we even started using a pattern of accepting additional keyword arguments to be passed directly as DOM attributes in the helpers (in addition to the explicit arguments). So, for instance:

<%= issue_badge state: issue.state.to_sym, id: 'some_id', 'data-foo': 'bar' %>

would render the Issues::Badge component and pass along the id and data-foo attributes onto the (root) rendered element.


I pulled two small examples from our app into rmacklin/components_in_rails (the helpers are in app/helpers/components) and rendered them in a very bare bones style guide: https://github.com/rmacklin/components_in_rails/blob/master/app/views/home/index.html.erb with these examples:

style_guide/_button_group_example.html.erb

<%= button_group do |button_group| %>
  <%= button_group.button('Cat in the Hat', active: true, class: 'js-custom-class foobar') %>
  <%= button_group.button do %>
    <span class="fa fa-pencil"></span> Bartholomew and the Oobleck
  <% end %>
  <%= button_group.button('Yertle the Turtle', :'data-foo' => 'bar') %>
<% end %>

style_guide/_help_bubble_example.html.erb

<%= help_bubble(
  'You will need to restart this move in',
  accompanying_text: 'Looking for a different property?',
  direction: 'ne',
  id: 'different_property_help_bubble'
) %>

Here are a few more examples that haven't been pulled into that repo (I wouldn’t mind sharing their implementations, though):

<%= options_dropdown label: 'Actions' do |dropdown| %>
  <%= dropdown.item 'Dropdown Item 1' %>
  <%= dropdown.item_link 'Dropdown Item 2', url: 'http://www.example.com' %>
<% end %>
<%= datapair key: 'Custom Class', value: 'Inspect Me', id: 'datapair_with_id', class: 'js-datapair' %>
<%= block_panel 'Personal Information', action_url: owners_path(@owner) do %>
  <%# Block body contents go here %>
<% end %>
<%= expandable_section title: 'title', initial_state: 'collapsed' do %>
  <div>Content</div>
<% end %>
<%= banner_alert title: 'Something', subtitle: 'I am a subtitle' do %>
  <%# ... %>
<% end %>

A more involved component looks like:

<%= filterable_collection do |fc| %>
  <%= fc.filter_box instructions: 'Enter in your filters below.' do %>
    <%= simple_form_for model.new, url: '/' do |f| %>
      <%# ... %>
      <% end %>
      <div class="btn-toolbar">
        <%= f.submit 'Search', class: 'btn btn-primary', :'data-disable-with' => 'Please Wait...' %>
      </div>
    <% end %>
  <% end %>

  <%= fc.results html_options: { id: 'results' } do %>
    <%# Initial results go here %>
  <% end %>
<% end %>

Some more complex components are backed by a view model rather than having a slew of different method signatures:

<%= paginated_table(id: 'people_table', view: @table_view) %>

Anyway, that’s probably more than enough examples to get the idea across. As I mentioned, a lot of our work is already quite similar, but this pseudo-custom-element syntax was one notable difference we tried and liked.

And this might be a crazy idea, but I think we could potentially autogenerate the helper methods either from iterating over ActionView::Component.descendants or maybe using an inherited hook, such that we could provide the psuedo-custom-element syntax as sugar over the render Component syntax. It’s late now, but maybe if that doesn’t sound crazy tomorrow I can experiment with it.

joelhawksley commented 5 years ago

@rmacklin this is all wonderful to see, and raises some questions/ideas that haven't come up before. Would you mind posting this on the Rails PR?

https://github.com/rails/rails/pull/36388

zachahn commented 5 years ago

I'm not sure if anyone tried this or if this is helpful at all, but I tried working on a non-monkeypatched version of this this past weekend. I ended up having some problems so I decided to stick with partials, but now that I think about it, it might have been because I didn't use the capture helper? I might need to try this again if I have some time lol 🤷‍♂️

Anyway, in case anyone is interested, the gist was something like the following

class ApplicationComponent
  def render_object(view_context, &block)
    # compile template, render template, etc
  end

  def to_partial_path
    "application_view_component/application_view_component"
  end
end

class MyComponent < ApplicationComponent
end
<%# application_view_component/_application_view_component.html.erb %>
<%= render application_view_component.render_object(self) %>
<%= render MyComponent.new %>

I didn't get this far, but I considered calling prepend_view_path on my controller along with a custom ActionView::Resolver (I found a couple gems that used that to find views that were located in the database, but I didn't look into it any further than that) so that I would need only one copy of the partial. In the project I was working on, I needed at least two copies of that erb partial lol, and I didn't think having more than two was very sustainable lol