Skip to content
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

WebP support #94

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from
Open

WebP support #94

wants to merge 5 commits into from

Conversation

patrocle
Copy link

@patrocle patrocle commented Aug 17, 2023

Questions Answers
Description? WebP support. WebP lossless images are 26% smaller in size compared to PNGs. WebP lossy images are 25-34% smaller than comparable JPEG
Type? improvement
BC breaks? no
Deprecations? no
Fixed ticket?
Sponsor company
How to test? Use the module. Upload a new image that uses WebP support. Without this PR, it does not work.

Copy link
Contributor

@matks matks left a comment

Choose a reason for hiding this comment

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

Hello @patrocle !

This is nice, however the current version of ps_imageslider is compatible with PrestaShop 1.7.4.0 until PrestaShop 8.1.0 (here https://github.com/PrestaShop/ps_imageslider/blob/dev/ps_imageslider.php#L65)

WebP image support has been added in PrestaShop 8.0.0 (see PrestaShop/PrestaShop#10356).

So if someone uses this PR on PrestaShop 1.7.8 for example it will not work.

I suggest that, in this PR, you modify the minimum version of PrestaShop compatible with ps_imageslider to 8.0.0 😉

@patrocle patrocle requested a review from matks August 18, 2023 09:38
Copy link
Contributor

@matks matks left a comment

Choose a reason for hiding this comment

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

Thanks for the changes 😉 I validate the PR. Let's now wait for QA team to test it.

Copy link
Contributor

@matks matks left a comment

Choose a reason for hiding this comment

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

Woups I was too quick 😅 we also need to remove the CI jobs that verify the module is compatible with prestashop <= 8.0.0

This is why the CI is failing: it attempts to verify the module works with PrestaShop 1.7.7, 1.7.8 ... but it's not relevant now

@patrocle patrocle requested a review from matks August 18, 2023 14:17
runs-on: ubuntu-latest
steps:
- name: Checkout
uses: actions/checkout@v2.0.0

- name: PHP syntax checker 5.6
Copy link
Contributor

@kpodemski kpodemski Aug 22, 2023

Choose a reason for hiding this comment

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

@patrocle I think it would be better to keep PHP checks for 7.2 to 8.1. As you probably know, PrestaShop 8.0 is compatible with PHP 7.2 to 8.1.

@kpodemski
Copy link
Contributor

Hey @patrocle

Thanks for your contribution!

A couple of suggestions:

  1. You have to create a config file for PHPStan: Project config file at path /var/www/html/modules/ps_imageslider/tests/phpstan/phpstan-8.1.0.neon does not exist.
  2. It would be better to check PHP compatibility between 7.2 and 8.1.

@@ -58,12 +50,12 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
presta-versions: ['1.7.4.4', '1.7.5.1', '1.7.6', '1.7.7', '1.7.8', 'latest']
presta-versions: ['8.0.0', '8.1.0', 'latest']
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we should use '8.0', '8.1' here (they mean 8.0.x and 8.1.x :)

@@ -35,7 +27,7 @@ jobs:
- name: Setup PHP
uses: shivammathur/setup-php@v2
with:
php-version: '7.4'
php-version: '8.1'
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused about the impact of this php-version change 😕

@@ -62,7 +62,7 @@ public function __construct()

$this->displayName = $this->trans('Image slider', [], 'Modules.Imageslider.Admin');
$this->description = $this->trans('Add sliding images to your homepage to welcome your visitors in a visual and friendly way.', [], 'Modules.Imageslider.Admin');
$this->ps_versions_compliancy = ['min' => '1.7.4.0', 'max' => _PS_VERSION_];
$this->ps_versions_compliancy = ['min' => '8.0.0', 'max' => _PS_VERSION_];
Copy link
Contributor

Choose a reason for hiding this comment

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

module version need to be bumped to 4.0.0 accordingly

@seakrebel
Copy link

seakrebel commented Mar 2, 2024

My ps_imageslider v3.1.4 doesn't seem to accept .webp source images even though other settings are configured correctly and products, categories, etc. have .webp source images that also render as such.

PrestaShop v8.1.4

Screenshot 2024-03-02 at 12 45 38@2x

Screenshot 2024-03-02 at 12 47 31@2x

EDIT: works after manually applying the fix from @matks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Waiting for author
6 participants