[Bug 1434690] Use different favicon on staging and development #4650
Conversation
jinja2/base.html
Outdated
| {% elif settings.DOMAIN == 'localhost' %} | ||
| {% set favicon_suffix = '-local' %} | ||
| {% else %} | ||
| {% set favicon_suffix = '' %} |
There was a problem hiding this comment.
The specific cases are production and staging, the fallback should be the -local - see the previous version:
There was a problem hiding this comment.
@jwhitlock In current configuration, there are nothing specific for production. But there are specific case for dev and stage. So I thought to have specific case for dev and stage and fallback to production
There was a problem hiding this comment.
The point of the different favicon.ico is to have one that only appears in production, one that only appears in staging, and everywhere else gets the dev icon. It's not OK for the production icon to be the default.
settings.PRODUCTION_DOMAIN should probably be added, and PRODUCTION_URL should be built from that rather than PROTOCOL and DOMAIN. Research is needed to see why it was done this way.
Codecov Report
@@ Coverage Diff @@
## master #4650 +/- ##
=======================================
Coverage 95.32% 95.32%
=======================================
Files 260 260
Lines 23534 23534
Branches 1688 1688
=======================================
Hits 22434 22434
Misses 888 888
Partials 212 212Continue to review full report at Codecov.
|
|
@jwhitlock I have fixed the thing. It should be exclusive to staging and production, and inclusive for all other instance. |
jwhitlock
left a comment
There was a problem hiding this comment.
I checked locally, and I got the red development favicon. Looking at the code, I believe this will pick the right icon in staging and production.
There are favicons above this line in jinja2/base.html. We don't have alternate versions of these, so we don't want to customize for different domains for now.
I searched the code, and PRODUCTION_URL is unused outside of the settings, so 👍 to the change to PRODUCTION_DOMAIN.
Thanks @safwanrahman!
It seems like the most efficiant fix with current configuration.
@jwhitlock small PR, r?