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

Closes #794 display the smaller size on image library data #810

Merged
merged 40 commits into from
Mar 20, 2024

Conversation

Khadreal
Copy link
Contributor

Description

Display the smaller size of the image(avif)

Fixes #794

Documentation

User documentation

This will show the avif image size & percentage when available

Technical documentation

Explain how this code works. Diagram & drawings are welcomed.

Type of change

  • Enhancement (non-breaking change which improves an existing functionality).

New dependencies

N/A

Risks

List possible performance & security issues or risks, and explain how they have been mitigated.

Checklists

Feature validation

  • I validated all the Acceptance Criteria. If possible, provide sreenshots or videos.
  • I triggered all changed lines of code at least once without new errors/warnings/notices.
  • I implemented built-in tests to cover the new/changed code.

Code style

  • I wrote self-explanatory code about what it does.
  • I wrote comments to explain why it does it.
  • I named variables and functions explicitely.
  • I did not introduce unecessary complexity.

@Khadreal Khadreal changed the base branch from develop to feature/avif February 21, 2024 11:56
@Khadreal Khadreal requested a review from a team February 21, 2024 13:46
Copy link
Contributor

@remyperona remyperona left a comment

Choose a reason for hiding this comment

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

I think we should go further on this one, updating this method and other methods in the class that reference webp, to make it work with the new implementation (using next-gen). This would also require to update the interface.

@Khadreal
Copy link
Contributor Author

I think we should go further on this one, updating this method and other methods in the class that reference webp, to make it work with the new implementation (using next-gen). This would also require to update the interface.

If I understand correctly, we should update the method from webp to nextgen ?

@remyperona
Copy link
Contributor

Yes, update all occurrences of webp to nextgen. I see there is also a use of imagify_path_to_webp() that should be updated in this class for example.

@Miraeld Miraeld self-requested a review February 23, 2024 13:35
@Mai-Saad
Copy link

Mai-Saad commented Mar 3, 2024

@Khadreal Thanks for the PR. working as in trunk after merging main branch to the PR locally while image have only webp.

Note: we have some discussion about sizes here (probably it isnot always the smallest on trunk), however, the agreed expected shall be in separate GH as it's the case on trunk.

Update: as per slack discussion, we shall display here the size of avif , if not exist then webp, if not exist then optimized => now we display in media library new size as per enabled option. which doesn't meet this update from slack. @Khadreal can you please check?
steps:
1- avif enabled
2- upload image => avif created and new size =that of avif
3- disable avif => webp created
4- check media library => new size = webp size instead of avif
bird-1905255_1280

/cc @piotrbak

@piotrbak piotrbak removed this from the 2.2 milestone Mar 4, 2024
Base automatically changed from feature/avif to develop March 6, 2024 14:54
classes/Bulk/Bulk.php Outdated Show resolved Hide resolved
@Khadreal
Copy link
Contributor Author

This PR #851 once merge will fixed the failing test error

@MathieuLamiot
Copy link
Contributor

@Khadreal should this go back to ready for review, or there are still work needed here?

@Khadreal
Copy link
Contributor Author

@Khadreal should this go back to ready for review, or there are still work needed here?

I'll move it, done with the work but didn't move it because of the test failing

@Mai-Saad
Copy link

Mai-Saad commented Mar 14, 2024

@Khadreal Thanks for the update. Please find exploratory note below:

  • Can see the following error for medium size image
    [14-Mar-2024 12:29:32 UTC] PHP Warning: Undefined array key "optimized_size" in /home/mai/Local Sites/imagifynginx6/app/public/wp-content/plugins/imagify-plugin/classes/Optimization/Data/AbstractData.php on line 309
    in the following scenario:
    1- install and activate imagify
    2- upload med size image attached
    marchmed1
    3- enable avif

or
1- activate avif
2- upload the same image

Screenshot from 2024-03-14 14-24-53

can you please check?

Note: in general, related test case https://wpmediaqa.testrail.io/index.php?/cases/view/15271

@Khadreal
Copy link
Contributor Author

Got this error, wondering if you're seeing something like this as well. This is when avif is not enable.

Screenshot 2024-03-15 at 15 33 51

@Khadreal
Copy link
Contributor Author

#794

Got this error, wondering if you're seeing something like this as well. This is when avif is not enable.

Screenshot 2024-03-15 at 15 33 51

My last update fixed this too

@Khadreal Khadreal requested a review from remyperona March 15, 2024 16:02
@Mai-Saad
Copy link

@Khadreal Thanks for the update, fixed in both scenarios mentioned above but can see problem if we refresh admin after scenario 2 i.e
1- install and activate imagify
2- enable avif
3- upload the image => avif created and no error
4- refresh media page , the warning is there

in log can see
[18-Mar-2024 09:52:12 UTC] PHP Warning: Undefined array key "full@imagify-webp" in /home/mai/Local Sites/imagifynginx6/app/public/wp-content/plugins/imagify-plugin/classes/Optimization/Data/AbstractData.php on line 314

note: it happens with any optimized image not just the previous one
screen-capture (88).webm

@Khadreal
Copy link
Contributor Author

Fixed. please check again.
Sorry for the back and forth

@Mai-Saad
Copy link

@Khadreal Thanks for the update. Working fine now with the following note (which is fine as per https://wp-media.slack.com/archives/CU0F6EGQ1/p1709546156479809?thread_ts=1709458320.636499&cid=CU0F6EGQ1)

  • if we manually deleted next-gen, the displayed size won't change even after refresh media page (i.e displayed size is that of avif, then manually delete the avif, the displayed size still is that of avif)

@Mai-Saad Mai-Saad added this pull request to the merge queue Mar 20, 2024
Merged via the queue into develop with commit 3b16931 Mar 20, 2024
5 checks passed
@Mai-Saad Mai-Saad deleted the avif/media-library#794 branch March 20, 2024 08:56
@remyperona remyperona added this to the 2.2.1 milestone Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

In media library New image size shall be that of AVIF|webP|optimized based on which exists
8 participants