Skip to content

Add configuration for and handling of other image types #3

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

Merged
merged 16 commits into from
Mar 28, 2025

Conversation

FinnReinhardtBsc
Copy link
Collaborator

@FinnReinhardtBsc FinnReinhardtBsc commented Mar 25, 2025

  • Add multiselect to enable / disable specific image types
  • Adjust config to account for different image types
  • Add additional config for new image types (and filter image ids accordingly)
  • Adjust logic to account for new image types

Based on changes from !2

@FinnReinhardtBsc FinnReinhardtBsc self-assigned this Mar 25, 2025
@FinnReinhardtBsc
Copy link
Collaborator Author

Hello @pykettk,
these are the changes that I added after merging your pull request 🙌 I'd like for you to take a look at them. Let me know what you think about my solution and if you would change anything. I am overall very pleased with how it turned out 😇

I have thoroughly tested it and all my tests were successful ✅

@pykettk
Copy link
Contributor

pykettk commented Mar 26, 2025

@FinnReinhardtBsc Looks good to me. I think the only things I'd say aren't massively important:

  1. It'd be nice to type hint the $imageType arguments in System/ModuleConfig.php
  2. There's probably no need to use CDATA in the comment of the image_types field in system.xml
  3. Some of the system.xml fields have had canRestore set to 0 despite there being entries for those fields in config.xml. Providing sensible defaults and allowing merchants to revert to using them is a nice quality of life feature IMO.

@FinnReinhardtBsc
Copy link
Collaborator Author

Thanks for your review, @pykettk!

  1. It'd be nice to type hint the $imageType arguments in System/ModuleConfig.php

Good eye, I added the missing types there! 👍

  1. There's probably no need to use CDATA in the comment of the image_types field in system.xml

I removed CDATA for the comment field.

  1. Some of the system.xml fields have had canRestore set to 0 despite there being entries for those fields in config.xml. Providing sensible defaults and allowing merchants to revert to using them is a nice quality of life feature IMO.

I see your point. When configuring this extension, I personally found it rather annoying to always uncheck the "use system value" box. As I see it, the default for the selected image types and resize mode provide less value than the other defaults do, because it depends a lot on your system.

But I understand that it is a good quality of life feature to have, so I changed them again :)


I also removed the "Only works on Hyvä Storefronts!" text (as mentioned here). My testing showed, that this functionality is not limited to Hyvä-Themes (although I would always recommend people to use Hyvä, of course :D). I just wanted to hear your take on this, in case I missed something.

Thanks for your review, @pykettk!
If there aren't any other issues, I will merge the changes and create a new release 🎉

@pykettk
Copy link
Contributor

pykettk commented Mar 27, 2025

I also removed the "Only works on Hyvä Storefronts!" text

I think that was my bad - I copied part of one of our system.xml files when working on this and overlooked it 😅

Everything else looks good though! 💪

@FinnReinhardtBsc FinnReinhardtBsc merged commit 04c5df6 into main Mar 28, 2025
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