search

lookbook-hq / lookbook

A UI development environment for Ruby on Rails apps ✨
https://lookbook.build
MIT License
912 stars 94 forks source link

Fix rendering of partials in previews #606

Closed liram11 closed 7 months ago

liram11 commented 7 months ago

This PR adds a setting to the Lookbook config which allows disabling automatic prefixing of partials on the ActionView layer.

Motivation / Background

Rendering partials in Lookbook 2.2.2 using ActionView short-hand syntax render @users fails with the following error:

Missing partial lookbook/users/_user with {:locale=>[:en], :formats=>[:html], :variants=>[], :handlers=>[:raw, :erb, :html, :builder, :ruby, :jbuilder]}.

Searched in:
  * "/Users/liram/work/lookbook-partials-test/partials/test/components/previews"
  * "/Users/liram/work/lookbook-partials-test/partials/app/views"
  * "/Users/liram/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/lookbook-2.2.2/app/views"
  * "/Users/liram/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/railties-7.1.3.2/lib/rails/templates"
  * "/Users/liram/work/lookbook-partials-test/partials/app/views"
  * "/Users/liram/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/view_component-3.11.0/app/views"
  * "/Users/liram/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/lookbook-2.2.2/app/views"
  * "/Users/liram/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/turbo-rails-2.0.5/app/views"
  * "/Users/liram/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actiontext-7.1.3.2/app/views"
  * "/Users/liram/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/actionmailbox-7.1.3.2/app/views"

In the error above ActionView automatically adds a lookbook prefix to the partial name which makes no sense in the context of lookbook.

A similar issue was described in #560.

Details

This issue can be fixed by modifying the ActionView::Base.prefix_partial_path_with_controller_namespace ActionView setting during the rendering.

I noticed the same case with ActionView::Base.annotate_rendered_view_with_filenames in ActionViewAnnotationsHandler and decided to extract both cases into ActionViewConfigHandler class.

Additional information

The new option is disabled by default so it wouldn't affect the default lookbook behavior.

Also, I was thinking of adding some integrational tests based on the dummy app. But I decided not to do that for now because It may require adding a db layer to the dummy app (to test render @users case). Let me know if it can be valuable and I'll do some experiments in a separate PR.

netlify[bot] commented 7 months ago

Deploy Preview for lookbook-docs ready!

Name Link
Latest commit 8319d04613c8360e9e2381221a72e6bc1efb5398
Latest deploy log https://app.netlify.com/sites/lookbook-docs/deploys/661ff0b788fa430008c1a540
Deploy Preview https://deploy-preview-606--lookbook-docs.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

allmarkedup commented 7 months ago

Hey @liram11 this is fantastic, thank you so much for your work on this! And sorry I've taken a while to get to this.

I'm going to check it out in more detail now but at first glance it looks great. To be honest the only thing I'm unsure about is whether this should actually be enabled by default as I think the current behaviour is really a bug. Whilst in theory this is a potentially breaking change I'm not sure I can think of any real world example where people would be relying on the current behaviour - would you agree?

I'm tempted to default to enabling this by default whilst giving people the option to disable it if there is some reason they may need to.

Also, I was thinking of adding some integrational tests based on the dummy app. But I decided not to do that for now because It may require adding a db layer to the dummy app (to test render @users case). Let me know if it can be valuable and I'll do some experiments in a separate PR.

Honestly, I'm pretty embarrassed about the state of the tests and test setup at the moment. Any additions or improvements would be most welcome but you might find it a bit of an uphill struggle!

allmarkedup commented 7 months ago

@liram11 It looks like the tests are failing for Rails 6.0 (which doesn't have the ActionView::Base#annotate_rendered_view_with_filenames method) - do you think you could take a look at that? The code looks fine to me so I think it may just be to do with the way it's being tested.

liram11 commented 7 months ago

Hey @allmarkedup,

Thanks a lot for taking a look at that!

I'm tempted to default to enabling this by default whilst giving people the option to disable it if there is some reason they may need to.

I have the same feeling about that. To be honest, I was thinking about making this a default behavior but decided to get some initial feedback before introducing it. Since we agree on that - let me change it to be a default behavior.

It looks like the tests are failing for Rails 6.0

Sure, I'll take a look. I should have some time today.

liram11 commented 7 months ago

Honestly, I'm pretty embarrassed about the state of the tests and test setup at the moment. Any additions or improvements would be most welcome but you might find it a bit of an uphill struggle!

Understood, I'll do some experiments with that later and will share the results in case anything sensible comes out😉

allmarkedup commented 7 months ago

I have the same feeling about that. To be honest, I was thinking about making this a default behavior but decided to get some initial feedback before introducing it. Since we agree on that - let me change it to be a default behavior.

Fantastic - in future versions we may be able to remove the config option altogether but it's good to have that for now just in case.

Sure, I'll take a look. I should have some time today.

Thank you! Much appreciated 🙂

liram11 commented 7 months ago

Hey @allmarkedup,

I changed the default behavior of the new option - now it is enabled by default.

I also restructured the tests a bit. Now they should work fine even if some ActionView parameters are not available.

allmarkedup commented 7 months ago

@liram11 Fantastic, thank you - this is looking great now. And I appreciate you taking the time to update the config documentation to include this new option too.

I've just given it a spin and I can't see any issues so I'm going to merge this in and get it out in the next release. Thanks again for your contribution!

allmarkedup commented 7 months ago

@liram11 I've just released v2.3.0 that includes this change, much appreciated!

liram11 commented 7 months ago

Nice! I'll upgrade it in my project soon. Thank you, Mark!