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

🧹Components: Light syntactical sugar for finding Components #1212

Closed
wants to merge 1 commit into from

Conversation

zspencer
Copy link
Member

This is probably a premature optimization, but I hate writing the entire class name out in template files! I don't know why! It just bothers me!

@zspencer zspencer requested review from anaulin and a team March 11, 2023 20:30
Copy link
Member

@anaulin anaulin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is fine?

Personally, it feels like component(:card...) adds another layer of non-trivial indirection to the alternative, which would be CardComponent, making it easier for someone that's reading the code to see which class is being used and find the relevant code.

Could it be that it is the ...Component bit in the naming that feels too heavyweight? Could that be improved by adopting a different naming convention for the Component classes that doesn't include Component in the name? This might be a terrible idea, too, opening us to potential confusion with models/controllers. 🤷🏼‍♀️

Your patient, doctor.

- #1187

This is probably a premature optimization, but I hate writing the entire
class name out in template files! I don't know why! It just bothers me!
@zspencer zspencer force-pushed the components/extract-component-helper branch from b350a7c to b8e92f0 Compare March 11, 2023 22:41
@zspencer
Copy link
Member Author

Yea, I think it's a pretty tiny nub of indirection at the moment; but I am also hoping for a world where I can do something like render component(model) and have that render the OrderComponent if the model is an Order or the CartComponent if model is a Cart... But I think that's me preferring the wild and wacky world of rails naming magic.

@anaulin
Copy link
Member

anaulin commented Mar 22, 2023

@zspencer how are you feeling about this PR? Ok to close it, or do you want to merge it?

@zspencer
Copy link
Member Author

I have deprioritized it since you didn't seem keen and I don't have a compelling use-case yet.

@zspencer zspencer closed this Mar 23, 2023
@zspencer zspencer deleted the components/extract-component-helper branch March 23, 2023 01:29
@anaulin
Copy link
Member

anaulin commented Apr 15, 2023

Now that I've spent some more time with ViewComponents, I think I see where you were coming from here: having to do render Entire::Ass::LongComponent.new(....) every time is kinda annoying.

The thing that I'm less sure about is replacing entire class names with a symbol, because I worry that it will make it more confusing to try to find a component if the reader of the code doesn't know about this helper. But maybe that's overly cautious of me, folks can search for the component helper definition and figure it out?

We could go with what you have here, plus shoving the render call into the component helper, so one can just call:

<%= component :button , variant: :foo %>

(if we go down this route, maybe it should also be able to take a string, so that we can deal more easily with nested classes, e.g. "marketplace/tax_rate" -> Marketplace::TaxRateComponent)

Or we could go the more-typing-but-maybe-clearer version where we make a class method render on the ApplicationComponent, where we can do something like:

<%= ButtonComponent.render(variant: :foo) %>

Thoughts?

@zspencer
Copy link
Member Author

I've been exploring pulling out little factory helper methods into the Components themselves. This gives us a clear linkage between the class of the component being rendered while still keeping the erb light and tight.

It's more direct than a flyweight registry (like w/Furniture and Utilities), or the 'infer-the-constant/partial' that Rails leans into, and it gives us affordances for pulling common stuff into mixins or parent classes.

I do like pulling the render into the component! I sometimes trip over the "how do we pass the block for content?!" but I think that's navigable.

@anaulin
Copy link
Member

anaulin commented Apr 16, 2023

I tried adding this to ApplicationComponent:

  def self.render(**kwargs, &block)
    render(new(**kwargs), &block)
  end

But it doesn't work, because render is only accessible from helpers, and at the class level in the Component helpers are not available (and callingh.render or helpers.render also don't work, since helpers are not available at this point).

So if we want to do something like this, we would add it as a method to our ApplicationHelper. 🤷🏼‍♀️

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

Successfully merging this pull request may close these issues.

2 participants