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

Make BaseComponent & FieldComponent extendable #69

Open
Spone opened this issue Sep 16, 2021 · 5 comments
Open

Make BaseComponent & FieldComponent extendable #69

Spone opened this issue Sep 16, 2021 · 5 comments
Labels
nice to have We are not working on it but we're open to contributions

Comments

@Spone
Copy link
Collaborator

Spone commented Sep 16, 2021

Continued from #21

How can we customize BaseComponent and FieldComponent?

  • With a concern?
  • How to support multiple FormBuilder?
@Spone Spone added the discussion Let's discuss ideas and features label Sep 16, 2021
@nicolas-brousse
Copy link
Member

I suggest to remove #html_class and replace it with #configure_options(options). But we have to manage helpers that have html_options separated (https://github.com/pantographe/view_component-form/blob/2803648f9e7c8317f2322bdffe6608b7ebbb3777/app/components/view_component/form/select_component.rb).

Also for generic config, I think we may have a method like #configure_options(component) (or better name) that is call by components to combine config. Since the builder is passed to components, we may call that method easily.

@Spone Spone added this to the v0.2 milestone Sep 30, 2021
@Spone
Copy link
Collaborator Author

Spone commented Sep 30, 2021

After discussion, here is what we will do:

  • Add a #html_options method to ViewComponent::Form::BaseComponent, that accepts the options object returns a Hash of HTML options
  • Add a #component_html_options method to ViewComponent::Form::Builder that accepts the options object and a component instance, so it can be customized by the user (without redefining all components)
  • Remove .default_options from ViewComponent::Form:BaseComponent
  • Change combine_options! in ViewComponent::Form:BaseComponent to merge:
    • @default_options / @default_html_options (already implemented) from the Builder
    • #component_html_options from the Builder
    • #html_options from the Component

class MyCustomBuilder
  def component_html_options(component, options)
    if component.is_a?(Form::SelectComponent)
      options[:class] += " my-custom-form-field"
    end
    options
  end
end
module Form
  class SelectComponent < ViewComponent::Form::SelectComponent
    def options(options)
      { include_blank: true }.merge(options)
    end

    def html_options(html_options)
      html_options.tap do |options|
        options[:class] += " my-custom-class"
      end
    end
  end
end

@Spone Spone mentioned this issue Sep 30, 2021
3 tasks
@boardfish
Copy link
Contributor

This issue became relevant for me just now. My component classes all inherit from their ViewComponent::Form equivalents, but if there was some common behavior I wanted to share between all of them, currently I'd have to include a module on each of them. I think it could be argued that it makes things a bit more specific, but my mental paradigm of these components is that they're form components first, select/text/phone number components second, so it feels a bit backwards to be including what I'd otherwise inherit.

I wonder if what we really need here is to be able to create a new class that inherits from ViewComponent::Form::BaseComponent to add functionality, and configure view_component-form to have all its components inherit from said class. Devise does something similar with its mailer class, which you can configure to inherit from your own ApplicationMailer or any other class.

@nicolas-brousse
Copy link
Member

nicolas-brousse commented Oct 20, 2021

We have already thought of this way, but if we allow that, then I think we will not be able to have more than one builder per app.
Or maybe I have miss something.

@boardfish
Copy link
Contributor

I think it might be linked to #80 - depending on where the lookup is defined (at the ViewComponent::Form or the builder level), that solution might navigate this. If each builder can use different components, then multiple builders is certainly a possibility with the above.

@Spone Spone mentioned this issue Nov 12, 2021
25 tasks
@Spone Spone modified the milestones: v0.2, v0.3 Feb 23, 2022
@Spone Spone removed this from the v0.3 milestone Feb 2, 2023
@Spone Spone added nice to have We are not working on it but we're open to contributions and removed discussion Let's discuss ideas and features labels Feb 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
nice to have We are not working on it but we're open to contributions
Projects
None yet
Development

No branches or pull requests

3 participants