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

feat(image-jsx): automatic compression and transcoding for AVIF, WebP… #6161

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

xhcy1314
Copy link

@xhcy1314 xhcy1314 commented Apr 26, 2024

Automatic compression and transcoding for AVIF, WebP, and original formats.

Provides smaller image formats while ensuring proper display on low-end devices.

Overview

This commit enhances the image processing functionality. Now, the image tools automatically compress and transcode AVIF, WebP, and original formats, providing smaller image formats while ensuring proper display on low-end devices.

What is it?

  • Feature / enhancement
  • Bug
  • Docs / tests / types / typos

Description

This commit enhances the image processing functionality. Now, the image tools automatically compress and transcode AVIF, WebP, and original formats. It provides smaller image formats while ensuring proper display on low-end devices. This improvement contributes to enhancing user experience, particularly for users with lower-performance devices.

Use cases and why

    1. A single configuration allows selecting either AVIF, WebP, or PNG image formats. Choosing AVIF format will result in images not being displayed on devices that do not support AVIF.
    1. Similarly, selecting WebP format will result in images not being displayed on devices that do not support WebP.

Checklist:

  • My code follows the developer guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • Added new tests to cover the fix / functionality

…, and original formats

Provides smaller image formats while ensuring proper display on low-end devices.
Copy link
Member

@wmertens wmertens left a comment

Choose a reason for hiding this comment

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

The idea is good but then you also need to change the jsx output so that it uses <picture> instead of <img>, because srcset doesn't let you tell the browser what the type is

https://stackoverflow.com/a/67698595/124416

@PatrickJS PatrickJS added the STATUS-3: no response The issue / PR creator has not responded for a while label May 6, 2024
@PatrickJS PatrickJS marked this pull request as draft May 6, 2024 05:44
@xhcy1314
Copy link
Author

xhcy1314 commented May 7, 2024

The idea is good but then you also need to change the jsx output so that it uses <picture> instead of <img>, because srcset doesn't let you tell the browser what the type is

https://stackoverflow.com/a/67698595/124416

I'll work on implementing it and testing its effectiveness as soon as possible. Thanks for the reminder!

@xhcy1314 xhcy1314 marked this pull request as ready for review May 7, 2024 13:05
@xhcy1314 xhcy1314 force-pushed the feat-imagetools-config-enhance branch from 3bec36a to 9de4ac0 Compare May 7, 2024 13:18
@xhcy1314 xhcy1314 requested a review from wmertens May 7, 2024 13:20
@xhcy1314
Copy link
Author

这个想法很好,但是您还需要更改 jsx 输出,以便它使用<picture>而不是<img>,因为 srcset 不允许您告诉浏览器类型是什么很好,但是您需要更改 jsx 输出,以便它使用 而不是 ,因为 srcset 没有告诉浏览器类型是什么

https://stackoverflow.com/a/67698595/124416https://stackoverflow.com/a/67698595/124416

Is there anything else I need to do ?

@xhcy1314 xhcy1314 requested a review from a team as a code owner May 28, 2024 07:05
Copy link

netlify bot commented May 28, 2024

👷 Deploy request for qwik-insights pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 18a9422

@PatrickJS
Copy link
Member

@wmertens is this ready?

Copy link
Member

@wmertens wmertens left a comment

Choose a reason for hiding this comment

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

This looks good, but:

  • it is slightly breaking in terms of CSS, no? I'm not sure if this should wait for v2 or if a new major is enough. @PatrickJS ?
  • it would be great if we had some unit tests of the output, just provide fake imagedata and show what the result should be.

@PatrickJS
Copy link
Member

if we have unit/e2e tests we should be fine

@wmertens
Copy link
Member

wmertens commented Jun 1, 2024

I think we should make this configurable, and in V2 we enable it by default.

So right now, by default keep the img output as-is, but allow avif with picture via a setting.

What do you think @xhcy1314 ?

@xhcy1314
Copy link
Author

xhcy1314 commented Jun 3, 2024

I think we should make this configurable, and in V2 we enable it by default.

So right now, by default keep the img output as-is, but allow avif with picture via a setting.

What do you think @xhcy1314 ?

I think this approach is reasonable. By making this feature configurable, we can introduce the new format without disrupting the current workflow. Enabling it by default in V2 will reduce the cognitive load on users, allowing them to focus on their business code.

However, i need your suggestion on what to name this configuration key.

@wmertens
Copy link
Member

wmertens commented Jun 3, 2024

@xhcy1314 how about this:

  • make the default format webp again
  • add jsdoc to the format parameter to explain that you can set multiple formats separated by ;
  • if there are multiple formats, generate a picture, otherwise an img

@xhcy1314
Copy link
Author

xhcy1314 commented Jun 3, 2024

@xhcy1314 how about this:

  • make the default format webp again
  • add jsdoc to the format parameter to explain that you can set multiple formats separated by ;
  • if there are multiple formats, generate a picture, otherwise an img

Okay, I need some time to tweak this part of the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
STATUS-3: no response The issue / PR creator has not responded for a while
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants