Inline the attachment form for taxon icons #3932
Conversation
| @@ -20,7 +20,20 @@ | |||
| </div> | |||
| <% end %> | |||
|
|
|||
| <%= render "spree/admin/taxons/attachment_forms/#{f.object.attachment_partial_name}", f: f %> | |||
| <%= f.field_container :icon do %> | |||
There was a problem hiding this comment.
IMO It still needs to iterate over the attachment definitions, it is pretty common to add more than icon attachment to taxons, hero image, mobile hero image, brand logo, etc
There was a problem hiding this comment.
I'm not against that in principle, I just thought that if done it should have its own documentation/guide/tests with complete support.
On the same note might make sense to completely remove the icon from the taxon and let individual store add it if needed, allowing to manage the whole attachment configuration (e.g. pick sizes, attach a video instead, etc.).
In my experience it's always the case that a store has it's own rules for images and attachments.
@softr8 would any of these alternative work for you?
There was a problem hiding this comment.
hi folks
I'm jumping in as we've added a lot of different attachments to our taxon object and do not use the default icon.
So as @elia, I'm in favour of removing the icon from the core model. And maybe move it into an extension if some store use only that icon, but my guess is that it won't be used that much.
There was a problem hiding this comment.
I'm open to someone trying to change my mind, but it feels like removing support for multiple taxon attachments, even if not documented/explicitly supported, would effectively constitute a breaking change.
As soon as we release some kind of public API, we must assume that people are using it and we can't just pull the rug from under their feet, no matter how badly documented it is, or whether it was introduced intentionally or by mistake.
With that said, I agree that we should probably remove the default "icon" attachment altogether and simply let people define their own attachments — but I also think that should be considered a breaking change.
Curious to hear if anyone from @solidusio/core-team feels differently about this.
There was a problem hiding this comment.
I added the usual deprecation/configuration harness
I added the required deprecations 👍
elia
force-pushed
the
elia/inline-the-taxon-attachment-form
branch
from
a868fc3 to
9963a93
Compare
elia
force-pushed
the
elia/inline-the-taxon-attachment-form
branch
from
9963a93 to
56ebf01
Compare
| @@ -20,7 +20,24 @@ | |||
| </div> | |||
| <% end %> | |||
|
|
|||
| <%= render "spree/admin/taxons/attachment_forms/#{f.object.attachment_partial_name}", f: f %> | |||
| <% if Spree::Config.keep_legacy_backend_support_for_multiple_taxon_attachments %> | |||
There was a problem hiding this comment.
I don't really get why the preference is needed. I think deprecating the partial and those methods in the _attachment files is enough.
There's no explicit support for multiple taxon attachment anywhere else in the solidus codebase, and the attachment_partial_name was using "paperclip" even for active_storage.
elia
force-pushed
the
elia/inline-the-taxon-attachment-form
branch
from
56ebf01 to
b5baa36
Compare
|
We are going to backport this one since 2.11 is ActiveStorage friendly |
kennyadsl
added
Needs Backport
changelog:solidus_backend
Description
There's no explicit support for multiple taxon attachment anywhere else in the solidus codebase, and the attachment_partial_name was using "paperclip" even for active_storage.
Given attachment adapters are substantially dormant prior to Rails 6.1 we just need to explain the change in the changelog without the burden of an overcomplex deprecation cycle.
Original PR: #3308
Checklist: