OCA / webkit-tools

https://odoo-community.org/project/tools-maintainers-30
GNU Affero General Public License v3.0
11 stars 71 forks source link

7.0 remove ref to "camptocamp_logo" #11

Closed gurneyalex closed 9 years ago

gurneyalex commented 10 years ago

and use a more neutral 'company_logo' name (backported from 8.0 PR)

eLBati commented 10 years ago

:+1:

coveralls commented 10 years ago

Coverage Status

Coverage remained the same when pulling f928f9e3036b1a191afca0e3f9cd7d182270c83c on gurneyalex:7.0 into d560ce5b4350ca68caa86f7c4fe9a3cbb570d51a on OCA:7.0.

nbessi commented 10 years ago

@pedrobaeza your option is a possibility else you have to create a new logo likle https://github.com/odoo/odoo/blob/8.0/addons/report_webkit/data.xml#L168.

The better solution will probably to patch the 8.0 branch of odoo and rename it as logo.

For the record, the header image where done as a requierment to be able to have conditional image/media in header and footer that can be managed in front end.

nbessi commented 10 years ago

That said creating a new logo will probably be less penalizing during migration process

pedrobaeza commented 10 years ago

You don't need to create any logo via XML. If you have a logo defined in your company, the expected result is to be used in any webkit report as default, as done in the other reporting engines (old RML, QWeb, and so on). If you want to use another logo, then webkit logos and XML it's the way to go.

nbessi commented 10 years ago

My concern about this is that it may break installation where user simply changed image in default image header. I'm not sure we can accept this PR in Version 7.0 . At least not without an announcement as it may break some installed configuration.

lepistone commented 10 years ago

I see your point @nbessi. Probably we should stand still on v7 (i.e. reject this PR) and do as @pedrobaeza suggests on v8. What do you think?

pedrobaeza commented 10 years ago

I don't think it breaks anything, because XML has <data noupdate="1"> attribute, so existing installations will remain the same.

nbessi commented 10 years ago

@pedrobaeza yes but wiht ${helper.embed_image('', company.logo) | n} it will use an other image the one defined in company not the one defined in the header image that was my point

pedrobaeza commented 10 years ago

But as header definition is not going to change on existing installations because the noupdate flag, there's no continuity problem. Remember that ${helper.embed_image('', company.logo) | n} is inside a field. It's not code, it's data.

nbessi commented 10 years ago

@pedrobaeza you ve got a point, but his modification will induce different behavior between default header and base report_webkit header. It is not a major issue but a pointer in manifest will be required in this case

pedrobaeza commented 10 years ago

OK, I agree in expanding manifest description to point this.

@gurneyalex, can you please do it?

nbessi commented 10 years ago

@gurneyalex @pedrobaeza as said in V8 PR you also have to have to ensure default logo dimentions will not break header. We must also specify an height and a width by default in embeed_image

nbessi commented 10 years ago

any news on this one

arthru commented 10 years ago

:+1: for the change suggested by @pedrobaeza

pedrobaeza commented 10 years ago

Sorry for not answering before, but I didn't see the comment.

First of all, better to use this helper method: ${helper.embed_company_logo()|safe}, that already makes all the tricks.

Secondly, the size is not going to change if your logo has the same resolution at the one hardcoded on the header, and I think we shouldn't restrict to any size, because logo tipologies are very different (some landscape, some vertical...).

guewen commented 9 years ago

ok for you @pedrobaeza?

gurneyalex commented 9 years ago

sorry for taking so much time to fix this :bow:

pedrobaeza commented 9 years ago

Yeah, thanks :+1:

legalsylvain commented 8 years ago

Hi all, it seems that this function _embed_companylogo doesn't exist in V7 serie. Or maybe I'm wrong ? I so backport the function in OCB. https://github.com/OCA/OCB/pull/450 kind regards.