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

[12.0] [MIG] web_widget_image_url #1145

Merged
merged 3 commits into from
Jun 19, 2019

Conversation

anandkansagra
Copy link
Member

Issue Reference

@anandkansagra anandkansagra mentioned this pull request Dec 31, 2018
40 tasks
@pedrobaeza pedrobaeza added this to the 12.0 milestone Dec 31, 2018
Copy link
Member

@tarteo tarteo left a comment

Choose a reason for hiding this comment

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

Thanks!

placeholder: "/web/static/src/img/placeholder.png",
supportedFieldTypes: ['char'],

url(){
Copy link
Member

Choose a reason for hiding this comment

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

This is not the correct syntax, please check the jslint error.

var AbstractField = require('web.AbstractField');
var core = require('web.core');
var registry = require('web.field_registry');
var QWeb = core.qweb;
Copy link
Member

Choose a reason for hiding this comment

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

This is never used

// License LGPLv3.0 or later (https://www.gnu.org/licenses/lgpl-3.0.en.html).

odoo.define('web_widget_image_url.FieldImageURL', function (require) {
"use strict";
Copy link
Member

Choose a reason for hiding this comment

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

Please indent

<t t-name="FieldImageURL">
<span class="oe_form_field oe_form_field_image" t-att-style="widget.attrs.style">
<img t-att-src="widget.url()"
t-att-border="widget.readonly ? 0 : 1"
Copy link
Member

@tarteo tarteo Jan 8, 2019

Choose a reason for hiding this comment

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

The border attribute is deprecated in html5.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't get it. Could you please describe it more?

Copy link
Member

@tarteo tarteo Jan 9, 2019

Choose a reason for hiding this comment

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

The attribute border is deprecated, please remove it. You can use the style attribute (t-att-style) to add a border to the image instead. https://developer.mozilla.org/en-US/docs/Web/HTML/Element/img#Deprecated_attributes

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the explanation.

@@ -0,0 +1,56 @@
.. image:: https://www.gnu.org/graphics/lgplv3-147x51.png
Copy link
Member

Choose a reason for hiding this comment

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

Can you use the new split readme?

@anandkansagra anandkansagra force-pushed the 12.0-mig-web_widget_image_url branch 2 times, most recently from 50f6f2c to ac5cc00 Compare January 10, 2019 04:49
Copy link
Member

@yvaucher yvaucher left a comment

Choose a reason for hiding this comment

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

Just tested it with thousands of images in a list view.

Works well.

And for code review LGTM

@yvaucher
Copy link
Member

@tarteo can you update your review please?

Copy link
Member

@nikul-serpentcs nikul-serpentcs left a comment

Choose a reason for hiding this comment

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

nitpicking LGPLv3 --> LGPL-3

Code review LGTM 👍

Copy link
Member

@tarteo tarteo left a comment

Choose a reason for hiding this comment

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

The syntax is still incorrect (see my comments):
https://travis-ci.org/OCA/web/jobs/477666187#L608

@anandkansagra anandkansagra force-pushed the 12.0-mig-web_widget_image_url branch from ac5cc00 to a974387 Compare April 18, 2019 09:21
@anandkansagra anandkansagra force-pushed the 12.0-mig-web_widget_image_url branch from a974387 to 03c8d6a Compare April 18, 2019 09:42
$img.on('error', function () {
$img.attr('src', self.placeholder);
self.do_warn(
_t("Image"), _t("Could not display the selected image."));
Copy link
Contributor

Choose a reason for hiding this comment

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

this can be really annoying if by chance you use the widget on multiple images and you have more than one image missing (tree view for instance).
Out of the scope of this PR but I'd add a TODO in the roadmap.

Copy link
Member

@tarteo tarteo left a comment

Choose a reason for hiding this comment

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

Thanks!

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@yvaucher yvaucher merged commit 3d19b97 into OCA:12.0 Jun 19, 2019
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.

9 participants