Update image settings documentation #4245
Conversation
|
The same update to the guides should be made for the EdgeGuides, I found this PR which already includes these changes. |
nbelzer
force-pushed
the
image-settings#3700
branch
from
86462ce to
aea45af
Compare
There was a problem hiding this comment.
The developer guides also mention Paperclip as the reason for the system requirement of ImageMagick. Perhaps this is also outdated as, since we are using Active Storage by default, it is also possible to use libvips (as outlined in the Active Storage Requirements as a faster and less memory intensive replacement for ImageMagick). I believe we should therefore also update these requirements.
@nbelzer thanks. 👆 please mention this as well.
guides/source/developers/products-and-variants/product-images.html.md
Outdated
Show resolved
Hide resolved
|
@kennyadsl d739f87 includes an update to the requirements section that includes the changed requirements. I made sure to mention that the changes are related to the chosen image backend and that stores that are using paperclip are still referred to using ImageMagick. |
|
We might also want to change the following lines to instead include libvips instead of imagemagick in the example brew install command: The consideration on changing this line would be that it less likely that the user has libvips installed (compared to imagemagick). Therefore, we would potentially increase the amount of time it takes to get started with Solidus for new users. |
guides/source/developers/products-and-variants/product-images.html.md
Outdated
Show resolved
Hide resolved
nbelzer
force-pushed
the
image-settings#3700
branch
from
5d07cd2 to
17ea16d
Compare
|
I have cleaned up the git history. |
There was a problem hiding this comment.
Hey, @nbelzer!
Sorry for taking a look into it so late. I just left a minor suggestion I think it'd be great to integrate. Let me know if you have any concern 🙂
Introduce tests to verify the configuration options added in b1fa1b8. After trying many different options to have `Spree::Image` re-evaluate the newly set preferences dynamically, I found this working solution. As `Spree::Image` is used in other spec tests it might already be loaded, and therefore has already evaluated the previous preferences. In this solution I decided to make use of a test-specific class that is equivalent to `Spree::Image`, but uses a fixed include (instead of a dynamic one). Each test contains one of the test-specific classes with a fixed include that references one of the potential `image_attachment_module`s. By including the custom class in the test we get the ability to directly influence the `Spree::Config` values that will be used upon definition of the class. Using the `stub_spree_preferences` we can set these preferences, without influencing other tests in test suite. The downside of this approach is that we should reflect changes to the `Spree::Image` class in the custom classes within (if the changes affect related behavior).
Because Paperclip is no longer the default for stores starting with Solidus 3, this documentation change reflects the change in requirements related to that (at least related to the image processing). Libvips is an alternative for ImageMagick that can be used by ActiveStorage.
nbelzer
force-pushed
the
image-settings#3700
branch
from
17ea16d to
679af53
Compare
|
Thanks for the suggestion @waiting-for-dev, I think that is a way better approach, learned something new today! |
|
Thanks for your work, @nbelzer! |
Resolves #3700
Description
As outlined in my comment on #3700 the guides recommend the use of a code snippet to override the attachment styles used in
Spree::Image. However, b1fa1b8 introduced configuration options, that can be set fromSpree::Configin an initializer likeconfig/initializers/spree.rb.This PR introduces updated documentation for the developer guides to reflect both the introduction of this configuration and the newly introduced Active Storage backend for
Spree::Imagein Solidus 3.0.Spec Tests
To verify that this behavior is correct I have included a set of tests for both of the available backends for
Spree::Image.The developer guides also mention Paperclip as the reason for the system requirement of ImageMagick. Perhaps this is also outdated as, since we are using Active Storage by default, it is also possible to use
libvips(as outlined in the Active Storage Requirements as a faster and less memory intensive replacement for ImageMagick). I believe we should therefore also update these requirements.Checklist:
I have attached screenshots to this PR for visual changes (if needed)