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

[ADD] web_favicon #252

Merged
merged 7 commits into from
Apr 7, 2016
Merged

[ADD] web_favicon #252

merged 7 commits into from
Apr 7, 2016

Conversation

hbrunn
Copy link
Member

@hbrunn hbrunn commented Nov 5, 2015

closes #199

@hbrunn hbrunn added this to the 8.0 milestone Nov 5, 2015
@http.route('/web_favicon/favicon', type='http', auth="none")
def icon(self):
request = http.request
company = request.env['res.company'].search([], limit=1)
Copy link
Member

Choose a reason for hiding this comment

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

This could be in the else part of the following if

@dreispt
Copy link
Member

dreispt commented Nov 6, 2015

Thank you Holger, this is a great feature.
There are some comments to take care of and make mr.Linter happy, but other than that LGTM.


.. image:: https://odoo-community.org/website/image/ir.attachment/5784_f2813bd/datas
:alt: Try me on Runbot
:target: https://runbot.odoo-community.org/runbot/{repo_id}/8.0
Copy link
Member

Choose a reason for hiding this comment

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

Please replace {repo_id}

@eLBati
Copy link
Member

eLBati commented Nov 7, 2015

Thanks @hbrunn
Tested on runbot
👍 after the other remarks are solved

@hbrunn
Copy link
Member Author

hbrunn commented Nov 9, 2015

@dreispt @eLBati thanks for your reviews, I took most of your points

@dreispt
Copy link
Member

dreispt commented Nov 9, 2015

It's 👍 after Travis is green.

@hbrunn
Copy link
Member Author

hbrunn commented Nov 9, 2015

@dreispt thanks, please remove the needs fixing label then when appropriate

@eLBati
Copy link
Member

eLBati commented Nov 9, 2015

👍

Configuration
=============

Upload your favicon (16x16, 32x32 or 64x64 pixels, 16 colors) on the company
Copy link
Member

Choose a reason for hiding this comment

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

A bigger resolution than 64x64 can be also used (and it's advisable for Android or iOS webapps for example). Also 16 colors is an older specification. Now the range color can be wider. Some reference: http://stackoverflow.com/questions/2268204/favicon-dimensions

@pedrobaeza
Copy link
Member

I was making something similar for website icon, but being static, so this one is very interesting, but your approach is a bit limited, because it doesn't take into account new specifications for declaring webapps icon for iOS and Android (which can be very useful, specially with responsive UI). Look at my module that already declared this: https://github.com/serviciosbaeza/serviciosbaeza-odoo-addons/blob/8.0/website_favicon/views/website_favicon.xml. You can also catch the nice icon I made: https://github.com/serviciosbaeza/serviciosbaeza-odoo-addons/blob/8.0/website_favicon/static/description/icon.png

I'm thinking of having multiple fields, one for each size, and the only required is the higher resolution one. Then, to have compute fields that catch the current resolution one if provided, and if not, a resized image to the proper size from the higher resolution image. All of this used in the links in the XML.

What do you think? Can you do it? May I propose the change to your branch?

@hbrunn
Copy link
Member Author

hbrunn commented Nov 9, 2015

@pedrobaeza propose it, I'll be happy to merge it

@dreispt
Copy link
Member

dreispt commented Nov 9, 2015

@pedrobaeza AFAICT the icon is cached and the "high" resolution is not that high. Maybe a single field would be just fine.

@pedrobaeza
Copy link
Member

@dreispt, you're thinking only in traditional browsers, but there are more applications.

@hbrunn
Copy link
Member Author

hbrunn commented Nov 13, 2015

@pedrobaeza is it a good idea to block this until we have the maximal version? Can't we merge now and add the fancy stuff whenever you find time to do it?

@dreispt
Copy link
Member

dreispt commented Nov 13, 2015

@hbrunn Put it in the Roadmap section of the README, as an idea for possible contributors..

@hbrunn
Copy link
Member Author

hbrunn commented Nov 13, 2015

@dreispt done

@dreispt
Copy link
Member

dreispt commented Nov 13, 2015

I think this is good to merge. @pedrobaeza ?

@pedrobaeza
Copy link
Member

No, please let me work on the points I have told.

@dreispt
Copy link
Member

dreispt commented Nov 13, 2015

Is there a problem on that going in a separate PR?

@pedrobaeza
Copy link
Member

Yes, the need for a migration script.

@hbrunn
Copy link
Member Author

hbrunn commented Feb 1, 2016

@pedrobaeza please either add the feature you want, or remove the work in progress tag. It's been two months now

@hbrunn
Copy link
Member Author

hbrunn commented Apr 6, 2016

I think this can be merged now, it doesn't seem @pedrobaeza will add the feature he want any time soon

@dreispt
Copy link
Member

dreispt commented Apr 7, 2016

@pedrobaeza can we merge?

@pedrobaeza
Copy link
Member

I'm preparing a quick PR with some of the changes we told (not all), and I'll propose it to @hbrunn's branch.

@pedrobaeza
Copy link
Member

You have in hbrunn#3 my little improvements. When I have time, I'll work in the rest of the roadmap.

[IMP] web_favicon: Nicer icon + README imp + support files
@hbrunn
Copy link
Member Author

hbrunn commented Apr 7, 2016

@pedrobaeza @dreispt merged

@coveralls
Copy link

Coverage Status

Coverage increased (+2.0%) to 62.5% when pulling dffd086 on hbrunn:8.0-web_favicon into ce57f98 on OCA:8.0.

@pedrobaeza
Copy link
Member

Travis is fine, so I merge. Thanks to all!

@pedrobaeza pedrobaeza merged commit 9ca4448 into OCA:8.0 Apr 7, 2016
@hbrunn hbrunn mentioned this pull request Apr 18, 2016
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.

5 participants