-
Notifications
You must be signed in to change notification settings - Fork 11
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
Adjust PLATFORM_NAME setting for Android app compatibility #30
Adjust PLATFORM_NAME setting for Android app compatibility #30
Conversation
This should be merged in either nightly or master. |
rebased to master |
PLATFORM_NAME: "{{ PLATFORM_NAME }}" | ||
# TODO: Temporarily set PLATFORM_NAME based on OPENEDX_COMMON_VERSION to 'OpenEdx' for compatibility with the Android app theme directory | ||
# (https://github.com/openedx/openedx-app-android expects 'OpenEdx'). This is addressed in PR https://github.com/openedx/openedx-app-android/pull/335, | ||
# which is merged into sumac.master and develop. Revert to "PLATFORM_NAME: '{{ PLATFORM_NAME }}'" once the pr changes are merged in master. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "master" branch of android app is actually "main". Please update the comment and then squash the commits.
@@ -68,7 +68,10 @@ BRANCH: | |||
ALTERNATE_HOST: '' | |||
|
|||
#Platform names | |||
PLATFORM_NAME: "{{ PLATFORM_NAME }}" | |||
# TODO: Temporarily set PLATFORM_NAME based on OPENEDX_COMMON_VERSION to 'OpenEdx' for compatibility with the Android app theme directory | |||
# (https://github.com/openedx/openedx-app-android expects 'OpenEdx'). This is addressed in PR https://github.com/openedx/openedx-app-android/pull/335, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(https://github.com/openedx/openedx-app-android expects 'OpenEdx')
Do you have a link that this is the value it expects? The actual issue is that platform name has spaces which cause issues with themeDirs. Therefore, the name is hardcoded to a value with no spaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR basically ensures theme works properly without being dependent on the PLATFORM_NAME value, which previously caused issues with theme directories.
Closing this PR as the changes from PR #335 have already been merged into the main branch. The issue with the 'FIXED' platform name has been resolved with this update. |
No description provided.