WL-307: correct FAVICON_PATH setting for comprehensive theming #11494
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -47,7 +47,7 @@ def comprehensive_theme_changes(theme_dir): | |||
|
|
|||
| favicon = component_dir / "static" / "images" / "favicon.ico" | |||
| if favicon.isfile(): | |||
| changes['settings']['FAVICON_PATH'] = str(favicon) | |||
| changes['settings']['FAVICON_PATH'] = "images/favicon.ico" | |||
There was a problem hiding this comment.
@saleem-latif I think overriding FAVICON_PATH setting in comprehensive theming does not make sense at all since we already have settings.FAVICON_PATH'] = "images/favicon.ico" in env/common.py. Themed directory has precedence over default static files path so it would look for images/favicon.ico in themed directory first before falling back to default static files directory. I think we should remove this favicon logic here.
5f7a8d8 to
d2dec33
Compare
|
@ziafazal I have removed favicon logic, kindly take a look |
|
LGTM 👍 |
|
Is it possible to include a test with this changeset? |
d2dec33 to
75f17ec
Compare
|
@mattdrayer I have added tests for change set, kindly take a look |
|
LGTM! thanks very much for adding the test -- 👍 |
saleem-latif
added a commit
that referenced
this pull request
Feb 16, 2016
WL-307: correct FAVICON_PATH setting for comprehensive theming
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi @ziafazal , @asadiqbal08
Kindly review this PR, it contains changes for WL-307,
Description of Changes:
Comprehensive theming sets
FAVICON_PATHsetting to a custom favicon but the value being set is not correct.Code here overrides
FAVICON_PATHsetting to the path of favicon in comprehensive theming directory.But instead of setting a relative path of the file it sets its absolute path. e.g. for a theme named stanford-style the
FAVICON_PATHsetting will be overridden by the following value./edx/app/edxapp/edx-platform/themes/stanford-style/lms/static/images/favicon.icoinstead of
images/favicon.icocc: @mattdrayer