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

7.0 remove ref to "camptocamp_logo" #11

Merged
merged 1 commit into from Sep 4, 2015

Conversation

Projects
None yet
10 participants
@gurneyalex
Member

gurneyalex commented Sep 8, 2014

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

@gurneyalex gurneyalex changed the title from change ref to camptocamp_logo to 7.0 remove ref to "camptocamp_logo" Sep 8, 2014

@eLBati

This comment has been minimized.

Show comment
Hide comment
@eLBati

eLBati Sep 8, 2014

Member

👍

Member

eLBati commented Sep 8, 2014

👍

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 8, 2014

Coverage Status

Coverage remained the same when pulling f928f9e on gurneyalex:7.0 into d560ce5 on OCA:7.0.

coveralls commented Sep 8, 2014

Coverage Status

Coverage remained the same when pulling f928f9e on gurneyalex:7.0 into d560ce5 on OCA:7.0.

@nbessi

This comment has been minimized.

Show comment
Hide comment
@nbessi

nbessi Sep 8, 2014

@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 Sep 8, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@nbessi

nbessi Sep 8, 2014

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

nbessi commented Sep 8, 2014

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

@pedrobaeza

This comment has been minimized.

Show comment
Hide comment
@pedrobaeza

pedrobaeza Sep 8, 2014

Contributor

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.

Contributor

pedrobaeza commented Sep 8, 2014

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 nbessi referenced this pull request Sep 10, 2014

Closed

8.0 base headers webkit #10

@nbessi

This comment has been minimized.

Show comment
Hide comment
@nbessi

nbessi Sep 10, 2014

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.

nbessi commented Sep 10, 2014

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

This comment has been minimized.

Show comment
Hide comment
@lepistone

lepistone Sep 10, 2014

Member

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?

Member

lepistone commented Sep 10, 2014

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

This comment has been minimized.

Show comment
Hide comment
@pedrobaeza

pedrobaeza Sep 10, 2014

Contributor

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

Contributor

pedrobaeza commented Sep 10, 2014

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

@nbessi

This comment has been minimized.

Show comment
Hide comment
@nbessi

nbessi Sep 10, 2014

@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

nbessi commented Sep 10, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@pedrobaeza

pedrobaeza Sep 10, 2014

Contributor

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.

Contributor

pedrobaeza commented Sep 10, 2014

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

This comment has been minimized.

Show comment
Hide comment
@nbessi

nbessi Sep 10, 2014

@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

nbessi commented Sep 10, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@pedrobaeza

pedrobaeza Sep 10, 2014

Contributor

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

@gurneyalex, can you please do it?

Contributor

pedrobaeza commented Sep 10, 2014

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

@gurneyalex, can you please do it?

@nbessi

This comment has been minimized.

Show comment
Hide comment
@nbessi

nbessi Sep 10, 2014

@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 Sep 10, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@nbessi

nbessi Sep 25, 2014

any news on this one

nbessi commented Sep 25, 2014

any news on this one

@arthru

This comment has been minimized.

Show comment
Hide comment
@arthru

arthru Oct 9, 2014

👍 for the change suggested by @pedrobaeza

arthru commented Oct 9, 2014

👍 for the change suggested by @pedrobaeza

@pedrobaeza

This comment has been minimized.

Show comment
Hide comment
@pedrobaeza

pedrobaeza Oct 16, 2014

Contributor

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...).

Contributor

pedrobaeza commented Oct 16, 2014

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...).

@gurneyalex gurneyalex added needs review and removed needs fixing labels Sep 4, 2015

@guewen

This comment has been minimized.

Show comment
Hide comment
@guewen

guewen Sep 4, 2015

Member

ok for you @pedrobaeza?

Member

guewen commented Sep 4, 2015

ok for you @pedrobaeza?

@gurneyalex

This comment has been minimized.

Show comment
Hide comment
@gurneyalex

gurneyalex Sep 4, 2015

Member

sorry for taking so much time to fix this 🙇

Member

gurneyalex commented Sep 4, 2015

sorry for taking so much time to fix this 🙇

@pedrobaeza

This comment has been minimized.

Show comment
Hide comment
@pedrobaeza

pedrobaeza Sep 4, 2015

Contributor

Yeah, thanks 👍

Contributor

pedrobaeza commented Sep 4, 2015

Yeah, thanks 👍

pedrobaeza added a commit that referenced this pull request Sep 4, 2015

Merge pull request #11 from gurneyalex/7.0
7.0 remove ref to "camptocamp_logo"

@pedrobaeza pedrobaeza merged commit 98b42bf into OCA:7.0 Sep 4, 2015

3 checks passed

ci/runbot runbot build 3114408-11-cfc331 (runtime 8s)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on 7.0 at 22.917%
Details

@yvaucher yvaucher removed the needs review label Sep 4, 2015

@legalsylvain legalsylvain added this to the 7.0 milestone Mar 9, 2016

@legalsylvain

This comment has been minimized.

Show comment
Hide comment
@legalsylvain

legalsylvain Mar 9, 2016

Hi all,
it seems that this function embed_company_logo doesn't exist in V7 serie. Or maybe I'm wrong ?
I so backport the function in OCB. OCA/OCB#450
kind regards.

legalsylvain commented Mar 9, 2016

Hi all,
it seems that this function embed_company_logo doesn't exist in V7 serie. Or maybe I'm wrong ?
I so backport the function in OCB. OCA/OCB#450
kind regards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment