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

Move Paperclip into an extension #2328

Open
wants to merge 39 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@swcraig
Contributor

swcraig commented Oct 25, 2017

Removes all Paperclip specific code (including all JavaScript related to uploading images in the admin) from Solidus. All of this functionality has been moved into a branch in (the currently temporary repo) solidus_paperclip.

Views in the admin/frontend render empty space for images. This way, future image uploading extensions only need to attach images to variants and they will show up in the expected places. Image upload implementations shouldn't need to know about specific views in Solidus.

This PR also adds #gallery methods to Spree::Variant and Spree::Product. These methods return an empty Enumerable, which future image upload extensions would be expected to override. I can open a separate PR for this change, but it is cleaner to have #gallery before this is merged. Most notably for JBuilder endpoints in the API and also the Handlebars templates in the admin. By using the null object pattern here I can leave those files without modification for now.

Spree::Taxon#icon has been removed, and a decorator has been added on the extension side. If this needs a better interface, I will work on addressing that.

This is a relatively large PR, and would be difficult to review easily. If there is an obvious way to split this up logically, please let me know.

I am more than happy to discuss any aspect of this change, and will do everything I can to move this forward in the best possible way.

Before this is merged solidus_paperclip needs to exist on solidusio-contrib and that location needs to be referenced in lib/sandbox.sh. At the moment it's a hardcoded relative path on my machine.

# This provides a more robust interface
#
# @return [Enumberable] of media for a variant
def gallery

This comment has been minimized.

@houndci-bot

houndci-bot Oct 25, 2017

Use empty lines between method definitions.

@@ -280,6 +266,16 @@ def find_variant_property_rule(option_value_ids)
end
end

This comment has been minimized.

@houndci-bot

houndci-bot Oct 25, 2017

Extra blank line detected.

# This provides a more robust interface
#
# @return [Enumberable] of media for a variant
def gallery

This comment has been minimized.

@houndci-bot

houndci-bot Oct 25, 2017

Use empty lines between method definitions.

@@ -280,6 +266,16 @@ def find_variant_property_rule(option_value_ids)
end
end

This comment has been minimized.

@houndci-bot

houndci-bot Oct 25, 2017

Extra blank line detected.

@swcraig swcraig changed the title from [WIP] Move Paperclip into an extension to Move Paperclip into an extension Oct 26, 2017

@jhawthorn jhawthorn self-assigned this Mar 28, 2018

@jhawthorn jhawthorn removed their assignment May 17, 2018

swcraig added some commits Sep 20, 2017

Remove shared image partial from backend
This partial renders product images.
Remove image related translations
These are used mostly in the upload zone, progress, and related areas.
Remove all JavaScript related to uploading images
This has been moved to solidus_paperclip. Other image upload extensions
will possibly need to borrow from there.

swcraig added some commits Oct 6, 2017

Remove the frontend shared partial for images
Extensions adding this would need to override this file.
Remove images from frontend views
Overrides have been added to the solidus_paperclip side.
Update README after pulling out Paperclip
Since we don't use Paperclip anymore, this isn't necessary.
Remove paperclip as a dependency of core
This has been moved into the solidus_paperclip extension.
Remove the images for the sample application
This also removes the code that loads the images.
Revise solidus_sample gemspec
We have taken all image (Paperclip) related content from Solidus.
Add solidus_paperclip to the sandbox
TODO: This is hardcoded path right now. This needs to point to an
actual gem before being merged.
Use `#gallery` in JBuilder templates
Otherwise I would need to add a weird "guard" statement here, and I
don't want to do that.

@tvdeyen tvdeyen added this to the 3.0 milestone Jun 13, 2018

@swcraig

This comment has been minimized.

Contributor

swcraig commented Jun 18, 2018

I think the best approach will be to merge #2337 before pulling out Paperclip.

@dankmitchell

This comment has been minimized.

dankmitchell commented Nov 5, 2018

Can this move forward now?

@ericsaupe

This comment has been minimized.

Contributor

ericsaupe commented Nov 7, 2018

@swcraig there are a number of conflicting files but this is a great direction!

Remove images from Spree::Product
Paperclip is dead and has been for months.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment