search

katalyst / koi

Koi Gem
https://github.com/katalyst/koi
MIT License
8 stars 4 forks source link

SummaryList attachment causes error when not an image by default #588

Closed mattr closed 5 months ago

mattr commented 5 months ago

99daecdb7468adf7c02f6585031e4d8496590448 updates the attachment declaration to expect an image attachment by default by specifying a variant: named parameter; however, this fails to account for non-image attachments.

def attachment(attribute, label: nil, skip_blank: @skip_blank, variant: :thumb, &)
  with_definition_attachment(@model, attribute, label:, skip_blank:, variant:, &)
end

Images are a special case of attachment -- I don't think the default implementation should make assumptions about this. I think it would make more sense to have a separate image component and declaration, as the rendering is always going to be different from other attachment types.

sfnelson commented 5 months ago

@mattr we've re-visited this in the latest build. Could you please review and give feedback?

mattr commented 5 months ago

@sfnelson Looks good. Not seeing the error after the latest update and removing variant: nil override.