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 Component suffix optional in lookup #80

Closed
boardfish opened this issue Oct 5, 2021 · 9 comments · Fixed by #162
Closed

Make Component suffix optional in lookup #80

boardfish opened this issue Oct 5, 2021 · 9 comments · Fixed by #162
Assignees
Labels
nice to have We are not working on it but we're open to contributions

Comments

@boardfish
Copy link
Contributor

It was previously confirmed that the Component suffix would remain a default in ViewComponent, but I'd appreciate the flexibility not to have to use it to keep in line with my existing schema for component names and namespaces.

@Spone
Copy link
Collaborator

Spone commented Oct 5, 2021

Hi @boardfish happy to see you here :)

How would you prefer view_component-form to handle this?

@boardfish
Copy link
Contributor Author

Hey there! 👋 Finally getting around to solving the problem of components and forms at @raisedevs, and it looks like the ideal solution's in the works right here. Nice work!

I'd imagine either by way of a config option on the builder or ViewComponent::Form, a manual lookup, or both. I did have the loose idea of a lookup config that's something like slots in ViewComponent itself, e.g:

lookup :text_field, SomeNamespace::TextFieldComponent
lookup :text_area, -> { |params| SomeNamespace::TextFieldComponent.new(type: :text_area, **params) }

The second example in particular is interesting because I'm sure some folks will want to use the same component as a base for several different fields. You could argue that that's also achievable with inheritance as has been done here in view_component-form, so perhaps it can be skipped as a concern.

@Spone
Copy link
Collaborator

Spone commented Oct 7, 2021

We could draw inspiration from Policy lookup in ActionPolicy, and offer a way to customize lookup logic as well.

@joelzwarrington
Copy link
Contributor

@Spone do you think I could work on this next?

Once #160 is released this would be relatively easy to implement in configuration.

@Spone
Copy link
Collaborator

Spone commented Apr 28, 2024

@joelzwarrington we'd be happy to review your contribution to implement this option!

@joelzwarrington
Copy link
Contributor

joelzwarrington commented Apr 29, 2024

Sounds good - I'll look at getting up a PR in the next day or so.


You previously mentioned using a similar approach to the lookup chain in Action Policy, is that still the case?

Something like this?

# in configuration...
lookup_chain = [
  lambda do |component_name, namespaces: []|
    namespaces.find do |namespace|
      "#{namespace}::#{component_name.to_s.camelize}Component".safe_constantize
    end
  end
]

and then used in ViewComponent::Form::Renderer#component_klass

def component_klass(component_name)
  @__component_klass_cache[component_name] ||= begin
    component_klass = ViewComponent::Form.configuration.lookup_chain.find do |lookup|
      lookup.call(component_name, namespaces: lookup_namespaces)
    end

    unless component_klass.is_a?(Class) && component_klass < ViewComponent::Base
      raise NotImplementedComponentError, "Component named #{component_name} doesn't exist " \
        "or is not a ViewComponent::Base class"
    end

    component_klass
  end
end

thoughts on this?

@joelzwarrington
Copy link
Contributor

hey @Spone what do you think? would you be onboard with that approach?

would be nice to be aligned before I whip something up

@Spone
Copy link
Collaborator

Spone commented May 22, 2024

👋 @joelzwarrington sorry about the late reply! Your approach sounds solid, I think you can move forward with it!

@joelzwarrington
Copy link
Contributor

No worries! I'll go ahead with my approach then. 👍

feel free to assign this issue to me if you'd like

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

Successfully merging a pull request may close this issue.

3 participants