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

[16.0][ADD] web_widget_many2many_binary_preview #3041

Open
wants to merge 1 commit into
base: 16.0
Choose a base branch
from

Conversation

hbrunn
Copy link
Member

@hbrunn hbrunn commented Dec 26, 2024

This module allows developers to enable a preview on fields displayed
with widget many2many_binary.

Easiest way to test this is to edit the view of mail templates and add the attribute show_preview="True" which I just did on runboat

@hbrunn hbrunn force-pushed the 16.0-web_widget_many2many_binary_preview branch from 51849d2 to dd65f80 Compare December 26, 2024 16:01
@@ -0,0 +1 @@
To use this module, you need to set the attribute `show_preview` to ``True`` on a field with widget `many2many_binary`.

Choose a reason for hiding this comment

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

I like preview better than show_preview, but ah well.

t-att-src="getUrl(file.id)"
aria-label="Download"
download=""
/>

Choose a reason for hiding this comment

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

As you already mentioned, the image previews can be rather sizy as compared to the more thumbnaily ones shown in the chatter attachment widget. I've noticed that this this is because of the URL; while here, getUrl returns web/image/XXX?download=true, in the other widget, it returns /web/image/XXX/1920x160 which returns the thumbnail. Maybe you can inject another getUrl and do the same here as well. I don't think multiple sizes need to be supported at the moment, but it could be a roadmap item to provide parameters for that.

<templates>
<t
t-name="Many2ManyBinaryField.attachment_preview"
t-inherit="web.Many2ManyBinaryField.attachment_preview"

Choose a reason for hiding this comment

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

Weird that the parent attachment_preview template does not in any way interface with the set of more advanced attachment preview components that are in the mail module. Maybe that's a pending refactor that didn't make the 16.0 release.

Choose a reason for hiding this comment

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

I'd almost think that it's better to make a new widget based on those components, since they have all kinds of size logic already. But that will probably open other cans of worms, so maybe just copy some of the logic to a new getPreviewUrl function and be done with it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe that's a pending refactor that didn't make the 16.0 release.

I don't think so

But that will probably open other cans of worms

None that I see, it will just be much more work to migrate this back and forth as the mail components have been refactored/rewritten a lot in the past releases, while the many2many_binary widget has barely changed.

I suggest for version 1 we just use the /web/image controller and expose its parameters (crop, width, height), that will already be very useful. Whenever inline preview should be implemented, we can look into using the mail components for that, but then we're already pretty much on attachment_preview territory and could just make that support images on many2many_binary fields.

Choose a reason for hiding this comment

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

Agreed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants