Skip to content

Conversation

@pykettk
Copy link
Contributor

@pykettk pykettk commented Mar 24, 2025

Description

Add some admin configuration options to control the image dimensions when resizing and provide a fallback (configurable via di.xml) in case of missing dimensions data.

@FinnReinhardtBsc
Copy link
Collaborator

Hello @pykettk,
thanks so much for your contribution, it is greatly appreciated! 🧡 You solved the task that I had the most uncertainty about - retrieving the values from the view.xml files.

I believe your changes are a great foundation to continue development. I have tested it locally and confirmed that this solution works 🙌 There is a little thing that I will highlight in a comment, but I would suggest to proceed as follows:

  • I will merge this branch into main
  • Based on your changes, I will create a pull request with additional features
    • add a system.xml configuration for the other image types
    • add virtual types to filter out the relevant images in the view.xml (e.g. only showing the images of type thumbnail for setting the thumbnail images
    • adjust the logic to consider each image type
  • I will request review from your side

My idea is to add a multiselect field which is used for enabling / disabling the image types. What do you think about that approach? This would allow for easy configuration regarding the image types that should be resized.

<label>Resize Mode</label>
<comment>
<![CDATA[
Only works on Hyvä Storefronts!
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it works best for Hyvä Storefronts - but this implementation is not logically theme dependent. I have tested locally, that the $viewConfig seems to always retrieve the aggregated data of the theme that is selected for the default website. I confirmed that it also works for Luma.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants