komposable / komponent

An opinionated way of organizing front-end code in Ruby on Rails, based on components
http://komponent.io
MIT License
426 stars 31 forks source link

adds all properties passed to component through helper method #65

Closed gkemmey closed 6 years ago

gkemmey commented 6 years ago

Seems like it'd be nice to get access to all the properties given to a component.

Usage Example

InputComponent

module InputComponent
  extend ComponentHelper

  property :f, required: true
  property :name, required: true

  def options
    @properties.slice(:f, :name).merge({
      class: (Array(properties[:class]) + ["text-input"]).join(' ')
    })
  end
end

_input.html.erb

<%= @f.text_field @name, options %>

_new_todo.html.erb

<%= form_with model: Todo.new do |f| %>
  <%= component 'input' f: f, name: :tile, id: "new-todo",
                                           placeholder: "What needs to be done?",
                                           autofocus: true,
                                           autocomplete: "off" %>
<% end %>

InputComponent becomes a wrapper around a call to text_field that you could apply some stylings to or whatever.

Testing

I wrote some tests that involved adding another app under fixtures called test_app (hence all the files). On that, I created a couple components I could use to test expected rendering in a ActiveSupport::ViewTest.

Looks like some of the testing process is in limbo in other PRs, so happy to adjust if need be.

Spone commented 6 years ago

Hi @gkemmey thanks for the PR! We'll review this with the team.

First thing that comes to my mind: I think we would need to "blacklist" the use of properties as a variable passed to the component helper, to make sure there is no risk of collision.

In other terms, we would raise an exception in case you write:

= component "input", properties: {}

so it doesn't conflict with using @properties.

Another approach would be to access properties using a properties method, instead of a @properties instance variable.

I think I prefer this second approach. What do you think?

gkemmey commented 6 years ago

Ah, makes sense. I agree that's nicer. So are you thinking

define_singleton_method(:properties) { locals }

would work instead?

florentferry commented 6 years ago

Hello @gkemmey,

Great idea to make the same variables available through properties or via instance variables. The method properties seems good to me.

Can you reuse the existing fixtures/my_app to make your tests instead of creating a new one ? I don't care between cucumber, rspec or test-unit, take what you like to make your tests. The 2 related PRs can give you inspiration for adapting fixtures/my_app in order to make tests working.

We should use the foundation you made to implement other tests for component_helper.

63 #32

gkemmey commented 6 years ago

@florentferry, @Spone Thanks for reviewing!

Think this is fixed, let me know if anything else needs to change.

florentferry commented 6 years ago

Thanks for the changes. Good to merge for me, just need a mention about the locals accessible via properties in https://github.com/komposable/komponent#passing-variables, and an addition to changelog.

Spone commented 6 years ago

Thanks for the changes, ready to merge!