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.

Sign up for GitHub

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

Default favicon #406

Merged
merged 5 commits into from
Sep 26, 2018
Merged

Default favicon #406

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0103ee5
Add default dash favicon.
T4rk1n Sep 26, 2018
bb0d0c4
Add cache control header to default favicon.
T4rk1n Sep 26, 2018
11905d8
Fix favicon format indent.
T4rk1n Sep 26, 2018
d2a2752
Add assets favicon cache busting, clearer favicon tag format.
T4rk1n Sep 26, 2018
30e8f14
Update version and changelogs.
T4rk1n Sep 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions MANIFEST.in
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
include README.md
include LICENSE
include dash/favicon.ico
20 changes: 15 additions & 5 deletions dash/dash.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,9 @@ def add_url(name, view_func, methods=('GET',)):
'{}<path:path>'.format(self.config['routes_pathname_prefix']),
self.index)

add_url('{}_favicon'.format(self.config['routes_pathname_prefix']),
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this route be {}_favicon.ico? I feel like this would make it clearer that the route is serving a .ico file directly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes probably clearer thanks.

self._serve_default_favicon)

self.server.before_first_request(self._setup_server)

self._layout = None
Expand Down Expand Up @@ -461,11 +464,9 @@ def index(self, *args, **kwargs): # pylint: disable=unused-argument
config = self._generate_config_html()
metas = self._generate_meta_html()
title = getattr(self, 'title', 'Dash')
if self._favicon:
favicon = '<link rel="icon" type="image/x-icon" href="{}">'.format(
self.get_asset_url(self._favicon))
else:
favicon = ''
favicon = '<link rel="icon" type="image/x-icon" href="{}">'.format(
self.get_asset_url(self._favicon) if self._favicon
else '{}_favicon'.format(self.config.requests_pathname_prefix))
Copy link
Contributor

Choose a reason for hiding this comment

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

I found this logic with the nested formats and conditional expression a little hard to parse initially. I'd have a preference for something more verbose but easier to read, like:

if self._favicon:
    favicon_url = self.get_asset_url(self._favicon)
else:
    favicon_url = '{}_favicon'.format(self.config.requests_pathname_prefix)
favicon = '<link rel="icon" type="image/x-icon" href="{}">'.format(favicon_url)


index = self.interpolate_index(
metas=metas, title=title, css=css, config=config,
Expand Down Expand Up @@ -975,6 +976,15 @@ def add_resource(p, filepath):
def _invalid_resources_handler(self, err):
return err.args[0], 404

def _serve_default_favicon(self):
headers = {
'Cache-Control': 'public, max-age={}'.format(
self.config.components_cache_max_age)
}
return flask.Response(pkgutil.get_data('dash', 'favicon.ico'),
headers=headers,
content_type='image/x-icon')

def get_asset_url(self, path):
asset = _get_asset_path(
self.config.requests_pathname_prefix,
Expand Down
Binary file added dash/favicon.ico
View file
Open in desktop
Binary file not shown.