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 omero.web config option to set custom favicon #311

Merged
merged 4 commits into from
Sep 9, 2021

Conversation

emilroz
Copy link
Member

@emilroz emilroz commented Aug 24, 2021

This PR introduces a config option to overwrite webgateway/img/ome.ico with a custom icon.

Favicon can be overwritten by setting favicon_url option with:

omero config set omero.web.favicon_url 'omero_web_module_name/subdir/new.ico'

The URL to the favicon needs to be specified relative to the Django's static directory.

@@ -118,7 +118,7 @@ def redirect_urlpatterns():
urlpatterns += [
url(
r"^favicon\.ico$",
lambda request: redirect("%swebgateway/img/ome.ico" % settings.STATIC_URL),
lambda request: redirect("%s%s" % (settings.STATIC_URL, settings.FAVICON_URL)),
Copy link
Member

Choose a reason for hiding this comment

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

Would an absolute FAVICON_URL ever be in scope?

Copy link
Member Author

Choose a reason for hiding this comment

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

That would require additional nginx/webserver config if you want to do it yourself right?

Copy link
Member

Choose a reason for hiding this comment

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

@joshmoore You mean something equivalent to the top_links logic where we handle local vv absolute links:

try:
    link["link"] = reverse(link_id)
except NoReverseMatch:
    # assume we've been passed a url
    link["link"] = link_id

I guess for favicon_url you could test url.startswith("http") or something like that?

"omero.web.favicon_url": [
"FAVICON_URL",
'"webgateway/img/ome.ico"',
json.loads,
Copy link
Member

Choose a reason for hiding this comment

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

Should this not just be str? Is there a reason for this to be JSON encoded?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Member

Choose a reason for hiding this comment

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

You need to loose the quotes in your example in the PR description and the default now I believe.

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, done.

@will-moore
Copy link
Member

This works fine for me testing locally and code looks good. 👍
Good to merge.

@joshmoore joshmoore merged commit 7df49de into ome:master Sep 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants