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

Using SlotableDefault include does not allow default slot to be overridden #2169

Open
BillWatts opened this issue Nov 22, 2024 · 2 comments
Open

Comments

@BillWatts
Copy link

BillWatts commented Nov 22, 2024

SlotableDefault include is not allowing for rendering over the default slot method.

Steps to reproduce

  1. Include SlotableDefault in a component, add a slot, and then define a default method for that slot.
  2. Render the component and try to override the default by using with_[slot_name]

Expected behavior

The slot should be overridden with what is provided by with_[slot_name].

Actual behavior

The slot default is rendered instead.

I took the get_slot method from the include and added a line from the original get_slot method in ViewComponent::Slotable and that seems to have fixed the issue, but not sure if that's entirely the right fix.

Willing to submit a PR if someone can point me in the right direction.

def get_slot(slot_name)
    content unless content_evaluated? # <- Line added

    @__vc_set_slots ||= {}

    return super unless !@__vc_set_slots[slot_name] && (default_method = registered_slots[slot_name][:default_method])

    renderable_value = send(default_method)
    slot = ViewComponent::Slot.new(self)

    if renderable_value.respond_to?(:render_in)
      slot.__vc_component_instance = renderable_value
    else
      slot.__vc_content = renderable_value
    end

    slot
end

Backtrace: None

System configuration

Rails version: 8.0.0

Ruby version: 3.3.0

Gem version: 3.20.0

@reeganviljoen
Copy link
Collaborator

@BillWatts thanks for the feedback, @joelhawksley do you have an idea what could be causing this

@ozzyaaron
Copy link

I was also running into the same issue but worryingly my tests were not detecting this.

  it "should have a placeholder if no header is defined" do
    render_inline(component)

    expect(page.find("#detail-page-header")).to have_content("COMING SOON")
  end

  it "should render the header that is provided" do
    render_inline(component.with_header_content("Test Header Content"))

    expect(page.find("#detail-page-header")).to have_content("Test Header Content")
    expect(page.find("#detail-page-header")).not_to have_content("COMING SOON")
  end

The tests pass when defining a def default_header but when I'd load the page the header content was not being rendered, it was falling back to the default_header

<%= render(ProductPageComponent.new(@product)) do |page| %>
  <% page.with_header do %>
    Something
  <% end %>
<% end %>

This renders the content that is defined in default_header unless I make the change suggested by @BillWatts

Tests however pass even without the change.

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

No branches or pull requests

3 participants