lookbook-hq / lookbook

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

Support Phlex DSL on scenarios #585

Open stephannv opened 5 months ago

stephannv commented 5 months ago

Fixes: https://github.com/ViewComponent/lookbook/issues/584

I'm not sure if this is the best solution but it's working 😅

Instead evaluating the block on Lookbook::Preview context, it is evaluating on component context, so it can render other components and use Phlex DSL.

netlify[bot] commented 5 months ago

Deploy Preview for lookbook-docs canceled.

Name Link
Latest commit da802d900ef1460582d63d45d1acf16fd2dff261
Latest deploy log https://app.netlify.com/sites/lookbook-docs/deploys/65be3d3ed76f2600071a34b1
joeldrapper commented 5 months ago

This does make sense if you're using Lookbook to document Phlex components that you intend to render inside other Phlex components, however, that's not the behaviour you would get from rendering a Phlex component in another context such as an ActionView view.

Perhaps it should be a configuration option for this reason. 🤔

stephannv commented 5 months ago

This does make sense if you're using Lookbook to document Phlex components that you intend to render inside other Phlex components, however, that's not the behaviour you would get from rendering a Phlex component in another context such as an ActionView view.

Perhaps it should be a configuration option for this reason. 🤔

Yeah, I guess I can create another preview class, eg.: Lookbook side:

# lib/lookbook/phlex_preview.rb
module Lookbook
  class PhlexPreview < Preview
    def render(component, &block)
      super do
        component.instance_exec component, &block
      end
    end
  end
end
<!-- app/views/lookbook/previews/preview.html.erb -->
<% if defined?(Phlex::SGML) && @render_args[:component].is_a?(Phlex::SGML) %>
  <%= render @render_args[:component], &@render_args[:block] %>
...

User side:

class MyComponentPreview < Lookbook::PhlexPreview # instead of Lookbook::Preview
  def default
    render MyComponent.new do |c|
      c.title { "Hello" }
      h2 { "World" }
      render MyOtherComponent.new
    end
  end
end
joeldrapper commented 5 months ago

Having a different preview class for Phlex-context-previews feels better to me, but rather than instance-exec’ing the the block against the component being rendered, it would be better to have the preview itself be a Phlex::HTML object. That’s more consistent with reality.

It’s possible for components to override HTML elements.

class A < Phlex::HTML
  def template
    h1 { "Hello" }
    yield
  end

  def h1 = super(class: "from_a")
end

class B < Phlex::HTML
  def template
    render A.new do
      h1 { "World!" }
    end
  end
end

Rendering B should output the following because the first heading came from A and the second heading came from B.

<h1 class="from_a">Hello</h1><h1>World!</h1>

But if B was a preview object using instance_exec on A, it would output this instead

<h1 class="from_a">Hello</h1><h1 class="from_a">World!</h1>

We can’t make Lookbook::PhlexPreview inherit from Phlex::HTML because it needs to inherit from Lookbook::Preview, but the next version of Phlex will provide everything from Phlex::HTML as a module you can include instead. It’s probably worth waiting on that.

stephannv commented 5 months ago

Having a different preview class for Phlex-context-previews feels better to me, but rather than instance-exec’ing the the block against the component being rendered, it would be better to have the preview itself be a Phlex::HTML object. That’s more consistent with reality.

It’s possible for components to override HTML elements.

class A < Phlex::HTML
  def template
    h1 { "Hello" }
    yield
  end

  def h1 = super(class: "from_a")
end

class B < Phlex::HTML
  def template
    render A.new do
      h1 { "World!" }
    end
  end
end

Rendering B should output the following because the first heading came from A and the second heading came from B.

<h1 class="from_a">Hello</h1><h1>World!</h1>

But if B was a preview object using instance_exec on A, it would output this instead

<h1 class="from_a">Hello</h1><h1 class="from_a">World!</h1>

It's true, I could reproduce this problem now =(.

We can’t make Lookbook::PhlexPreview inherit from Phlex::HTML because it needs to inherit from Lookbook::Preview, but the next version of Phlex will provide everything from Phlex::HTML as a module you can include instead. It’s probably worth waiting on that.

Nice, I did this on my similar lib (Blueprint) and I think it's a great approach

Screenshot 2024-02-01 at 11 17 31 AM
allmarkedup commented 5 months ago

@stephannv thanks for taking the time to put this together. And @joeldrapper thanks for jumping in on the discussion with your expertise on this, much appreciated. I'm sadly not using Phlex on a day-today basis at the moment so forgive me if i'm a bit out of my depth with the details of what is being discussed here.

I'm certainly not opposed to having a Phlex-specific preview class to inherit from (or a module that can be included in preview classes for Phlex components) if that means previews can be written in a way that feels more natural for Phlex users.

FWIW, the sub-component render issue is actually also present for ViewComponent previews - to render a sub-component within a parent's component's block context you need to use the parent component's render method. You have to do something like:

def example_preview
  render ExampleComponent.new do |parent|
    parent.render ChildComponent.new
  end
end

This is how it works in ViewComponent's own preview system which Lookbook previews are based on (and aim to be 100% compatible with). It's definitely not ideal for all the reasons you mention in #584 but will likely have to stay like that (for VC component previews, anyway).

So is the general feeling here that it's worth waiting until Phlex::HTML (or the contents thereof) is made available as an module before taking this any further?

joeldrapper commented 5 months ago

So is the general feeling here that it's worth waiting until Phlex::HTML (or the contents thereof) is made available as an module before taking this any further?

I think that's our best option. It shouldn't be too long now.

cirdes commented 1 week ago

@stephannv helped me out, and I was able to make it work by using an initializer:

config/initializers/lookbook.rb

module Lookbook::PreviewOverrides
  # see https://github.com/ViewComponent/lookbook/issues/584
  def render(component = nil, **args, &block)
    if block
      super { component.instance_exec component, &block }
    else
      super
    end
  end
end

Rails.application.configure { Lookbook::Preview.prepend Lookbook::PreviewOverrides }

But I would love to see better Phlex support!